clone: Allow specifying termination signal

[kamalmarhubi]

DO NOT MERGE: proof of concept for comment

Fixes #343

[kamalmarhubi]

@tailhook @fiveop @posborne here's another take on a fix for #343, leaning on ops::BitOr and convert::{From, Into} for some ergonomic type safe stuff. It lets us write CLONE_UTS | SIGCHLD in clone() args, but in a type safe way.

This is a proof of concept for comment. I'd probably do some more refactoring here first. Eg, I'm considering proposing a newtype for signal numbers, which would be a separate PR that would come first.

[tailhook]

Looks fine if signal will be a new type (or'ing with c_int looks unclear)

If it will not be a new type it's better to have CLONE_UTS.with_signal(SIGCHLD) (or a better name)

[kamalmarhubi]

Build failures are for my doc example using APIs not present in older versions of Rust.

Looks fine if signal will be a new type (or'ing with c_int looks unclear)

Agreed. We've got #254 open for changing signal to be an enum in nix following from the convention introduced in #282. That would nicely fix the issue.

[posborne]

To ensure we do not overwrite the other bits, it may make sense to mask self.signal before or'ing.

[kamalmarhubi]

Good point. It may not be necessary if we use an enum for signals though.... will toy with it over the next few days.

[fiveop]

Yes it does. Because only the low 8 signals are allowed if I understand the manpage correctly.

[posborne]

I like it 👍 Agreed on other changes to go with this.

[kamalmarhubi]

Some discussion with @arcnmx in the IRC channel (need to get logs enabled there!):

<arc> The or<c_int> might be a bit too magic compared to something more
    rust-like CloneFlagsSignal::new(flag, signal)
<kamalmarhubi> yeah I'm a bit on the fence of how closely we should match man
    pages for stuff like this
<kamalmarhubi> in fact I have a blog post lamenting some things about process
    related interfaces
<kamalmarhubi> which have this fun "-1 is everything, <-1 is process group n, 0
    is my process group, > 0 is just that pid" thing going on
<arc> Or flags.with_signal(etc)
<arc> heh...
<kamalmarhubi> I feel like we can improve on that but we'd be breaking from the
    libc APIs more
<kamalmarhubi> enum ProcessArg { All, GroupOf { pid: pid_t }, Mine, Process {
    pid: pid_t } }
<kamalmarhubi> something like that
<kamalmarhubi> shows up in kill, waitpid, possibly elsewhere
<arc> maybe, but nix already has a convention of using strong types for things
    like bitflags, or known return values.
<kamalmarhubi> yeah
<kamalmarhubi> <3 nix :-)
<arc> So I don't know that "OR because that's what you do in C" makes it an
    actual part of the API
<kamalmarhubi> I'm writing a blog post at the moment about fork / kill and how
    nix helps you not hit http://rachelbythebay.com/w/2014/08/19/fork/
<kamalmarhubi> so this stuff is on my mind a bit
<arc> alternatively, just make clone() take flags: CloneFlags, signal:
    Option<_>
<arc> just because they're one argument in libc (which is probably for
    historical and API compatibility reasons) doesn't mean it needs to be in
    nix.
<kamalmarhubi> yeah that was @fiveop's approach in https://github.com/nix-rust/nix/pull/344
<arc> they're pretty disjoint values anyway.
<arc> well I need to disappear... I'll try to be around a bit next week.
<arc> my opinion on clone() is probably that a separate argument makes the most
    sense. adding it to Flags is a bit convoluted and I'm not sure there's a
    real argument for it besides "well C does it that way".
<arc> Depends on how rusty you think it is to use Config-esque structs as a
    replacement for Rust's lack of optional function arguments.
<arc> nix has a precendent of using Option<> for those though so...

[tailhook]

Well, let me add a little bit:

1. I think there will be only few direct users of clone(), so user-friendliness of the interface doesn't matter too much. There is unshare crate which provides user-friendly interface of most of the real functionality (and the original issue appeared by trying to update the unshare to the new nix)
2. People searching for the clone() function (not a user-friendly interface) will consult the linux man page to find out what all that options mean. Sorry for repeating this, but it's not a matter of "how function is defined in C", but rather of "how would you find the comprehensive documentation of the parameters".

I really believe that just using CloneFlags.bits() | SIGCHLD (i.e. return parameter type to c_int) is much better than a separate argument. If you want function with different parameters call it create_process or user_friendly_clone or just use the unshare crate.

[fiveop]

I have read all your comments and thought about it. Here is what I think:

For the low level API that perfectly matches the C API's there is libc. I do not think we have to provide 1:1 mappings of arguments to functions. In fact, we already do not do this in various places, and this is why I want this library. Otherwise I could just use libc. So I do not agree with @tailhook.

Given this first proposition, the remaining proposals given so far fall in two groups: Two separate arguments or one composite argument. I do not see an advante in using a composite argument. It seems like an artifical construct to match the C API argument count (again). Therefore, I support using two seperate arguments.

I would use one argument for the CLONE_ flag of type CloneFlags and one for the signal of type c_int and later on an enumeration.

I do not specify what enumeration, because I see two options, and have not made my mind up yet:

We could either use the enumeration for signals in general, that we plan to add. This would force us to check that the signal is in the valid range in order to provide a safe interface.

Or we could create a separate enumeration just for this parameter. We could add code for automatic conversion between the special and the general signal enumeration. On direction of the conversion would have to implement the range checks. However, a user could use the specialised enumeration, if he knows that the signal will be within range (for example because it is constant) and thus avoid the range check.

[arcnmx]

I think there will be only few direct users of clone()

hi I'm one of them

so user-friendliness of the interface doesn't matter too much. I really believe that just using CloneFlags.bits() | SIGCHLD (i.e. return parameter type to c_int) is much better than a separate argument

nix has a convention of using strongly-typed arguments for API contracts (and ranges of valid values and so forth) that are otherwise only specified in man pages. I don't think this situation is any different, or that there's any rule saying the prototypes must be identical. It's not about being user-friendly, it's about being clean and type-safe. nix is not liblibc, and it's never aimed to be. The composite argument approach just seems forced and not very rusty.

[posborne]

I've come around to the argument that in this case having a separate argument exposed from nix for signal/flags probably makes most sense. The combination of the signal/flags in the kernel interface (and libc by extension) appears to be purely an optimization.

In general, I believe that for nix it probably makes sense to expose bindings to the logical *nix interface rather than the "physical" one. In this case performing an OR between flags and a signal feels like we are coupling ourselves too closely to the physical interface exposed by the kernel at the expense of clarity.

[kamalmarhubi]

I've come around to the argument that in this case having a separate argument exposed from nix for signal/flags probably makes most sense.

Me too. Closing this, and moving discussion to #343.