implement signal BitOr and SigSet::from! #615

cyphar · 2017-06-03T17:43:13Z

This allows for far more idiomatic initialisation of SigSet without
having to declare mut variables, as well as mirroring the vec![]
semantics (though arguably SigSet is closer to a bitfield than a
vector).

let mut sigset = SigSet::empty();
sigset.add(Signal::SIGINT);
// versus
let sigset = sigset![Signal::SIGINT];

And the BitOr* operations futher make it much easier to idiomatically
work with SigSet and Signal, without having to use sigset![] and
SigSet::extend() everywhere.

let sigset_a = sigset![Signal::SIGINT];
let sigset_b = sigset![Signal::SIGTERM];
let mut sigset = SigSet::empty();
sigset.extend(&sigset_a);
sigset.extend(&sigset_b);
// or
let sigset = sigset![Signal::SIGINT, Signal::SIGTERM];
// versus
let sigset = Signal::SIGINT | Signal::SIGTERM;

Implements #609
Signed-off-by: Aleksa Sarai [email protected]

Susurrus · 2017-06-03T21:56:43Z

I'm not a huge fan of using .into() to convert a Signal to a SigSet. They're not really equivalent.

The only use case I see missing here is supporting nice initialization. What about something akin to the vec![] macro but for SigSet?

Susurrus · 2017-06-03T21:58:36Z

Additionally I'm not a fan of using BitOr to OR together things that aren't really bitfields. I think it's semantically confusing.

cyphar · 2017-06-03T23:45:37Z

I'm not a huge fan of using .into() to convert a Signal to a SigSet. They're not really equivalent.

I will admit that I'm very new to Rust so I'm not sure what the right semantics are (and am quite willing to learn on what the "right way" of defining interfaces is in the Rust ecosystem). Is std::convert::From::from similarly "bad"?

I think it's semantically confusing.

While the underyling interface isn't a bitfield (and is accessed through the super-clumsy libc interfaces), a SigSet is semantically a set so I'm not sure I agree that set operations shouldn't be defined on something that is a set. I don't know if you use use Python, but Python sets define unary operations such as | and so on. It's unfortunate that Rust called the trait BitOr because | is quite useful for other things 😉.

cyphar · 2017-06-03T23:46:57Z

What about something akin to the vec![] macro but for SigSet?

That could also work I guess, though now users will have to use #[macro_use].

Susurrus · 2017-06-04T22:05:18Z

I will admit that I'm very new to Rust so I'm not sure what the right semantics are (and am quite willing to learn on what the "right way" of defining interfaces is in the Rust ecosystem). Is std::convert::From::from similarly "bad"?

From and Into normally mean that the types are roughly equivalent. Like a RawFd and a u32, the datatypes are similar semantically. With a Signal and a SigSet, I don't like that you can into(), as they aren't equivalent. One is a container of another. I see it like how I don't think you should implement u32::Into<Vec<u32>> since I don't consider those equivalent. One is effectively a container and the other is an element.

Additionally the value of Into seems quite minimal. Your bitor and bitor_assign tests cover all use cases I can imagine and that you have provided.

One thing I'd like to see is that SigSet get some expanded documentation describing these various ways of initializing them like you mention in this comment thread. That should make sure that future users don't get confused on how to best generate a SigSet.

This is looking really good though, that's for taking this on!

cyphar · 2017-06-05T10:09:09Z

@Susurrus Done, PTAL?

Susurrus · 2017-06-05T14:25:49Z

src/sys/signal.rs

 #[derive(Clone, Copy)]
 pub struct SigSet {
    sigset: libc::sigset_t
 }

+/// sigset is a macro that allows for far more idiomatic initalisation of SigSet. Note that while


I don't think we should have a sigset![] macro at all. I don't see what it offers that the BitOr operator doesn't allow. And the ergonomics with macros in Rust right now are less than that of regular functions or operators. Let's go ahead and just remove it.

The downside is that it requires doing SigSet::empty() | Signal::SIGHUP in order to "promote" a Signal to a SigSet.

Yeah, that's a great point. I'm actually thinking that going back to implementing From<Signal> for SigSet to be the right solution. Then we can rewrite all functions that take a SigSet to instead take a T: Into<SigSet> instead.

Yeah, From<Signal> for SigSet is a very reasonable thing to do IMO here. Agreed that it would be better to not have that macro unless it is really required.

Alright, I'll go back to that. Do you want me to do the T: Into<SigSet> here or in a separate patchset?

Same patchset. It's fine to have the From impl and the conversion of functions to use it as another commit.

Susurrus · 2017-06-05T14:27:26Z

src/sys/signal.rs


 // Currently there is only one definition of c_int in libc, as well as only one
 // type for signal constants.
 // We would prefer to use the libc::c_int alias in the repr attribute. Unfortunately
 // this is not (yet) possible.
+


Please remove this line.

Also, Signal needs doc comments now that there's this new behavior where you can combine Signals into SigSets.

cyphar · 2017-06-06T19:22:27Z

Alright, I've updated this to have impl From<Signal> for SigSet. I've also added the Into<SigSet>, and I fixed the signalfd tests and documentation (which used to not even compile).

cyphar · 2017-06-06T19:23:26Z

I'm sad that you can't implement a trait for a generic type. It would be nice to be able to do

impl<T: Into<SigSet>> BitOr<T> for T { /* ... */ }

Susurrus · 2017-06-06T23:07:40Z

src/sys/signal.rs

+///
+/// ```
+/// # use ::nix::sys::signal::*;
+/// // Into and From promote a signal to a SigSet that has only the given signal inside it.


Please leave these comments out of the code example instead and separate these into two separate examples. It'll make it slightly easier reading. Something like:

/// Into and From promote a signal to a SigSet that has only the given signal inside it. /// ``` /// # use ::nix::sys::signal::*; /// let sigset_a: SigSet = Signal::SIGTERM.into(); /// let sigset_b = SigSet::from(Signal::SIGTERM); ///``` /// /// `SigSet`s of multiple signals can be created by using the `|` (`BitOr`) operator: /// ``` /// # use ::nix::sys::signal::*; /// let sigset = Signal::SIGTERM | Signal::SIGHUP; /// ``` /// /// Existing `SigSet`s can be extended using the `|` (`BitOr`) or `|=` (`BitAssign`) operator as well: /// ``` /// let mut sigset = SigSet::empty(); /// sigset |= Signal::SIGQUIT; /// ```

Susurrus · 2017-06-06T23:07:57Z

src/sys/signal.rs

@@ -218,11 +263,42 @@ pub enum SigmaskHow {
    SIG_SETMASK = libc::SIG_SETMASK,
 }

+/// SigSet represents a set of signals and is a thin wrapper around the libc APIs for sigset_t.


Please put all types in "`" like sigset_t, etc.

Susurrus · 2017-06-06T23:09:55Z

src/sys/signal.rs

+/// # Examples
+///
+/// ```
+/// # use ::nix::sys::signal::*;


Please simplify this example to one SigSet. Maybe you should just copy/paste the relevant examples from the Signal docs above.

Susurrus · 2017-06-06T23:10:13Z

src/sys/signal.rs

+/// let sigset_d = sigset_c | sigset_a;
+/// ```
+///
+/// The above would traditionally require far more finicking with SigSet::add, SigSet::extend and


I think the first sentence here doesn't add anything, let's remove it.

Susurrus · 2017-06-06T23:11:36Z

src/sys/signalfd.rs

@@ -60,10 +60,10 @@ pub fn signalfd(fd: RawFd, mask: &SigSet, flags: SfdFlags) -> Result<RawFd> {
 /// # Examples
 ///
 /// ```
+/// use nix::sys::signal::*;


Let's make these use lines not appear in the docs by prepending with # since we're modifying this example.

Susurrus · 2017-06-06T23:13:38Z

test/test_signalfd.rs

@@ -11,8 +11,7 @@ use nix::unistd;
 fn main() {
    print!("test test_signalfd ... ");

-    let mut mask = signal::SigSet::empty();
-    mask.add(signal::SIGUSR1).unwrap();
+    let mask: signal::SigSet = signal::SIGUSR1.into();


We should allow a Signal to be passed directly into SignalFd::new() and related functions. Could you add that?

I tried to figure out how to do this and I've bumped up against the edges of my knowledge of Rust. Because SignalFd::new (and the other various methods for SignalFd) take &SigSet, we can't easily do something like this:

let sigmask = Signal::SIGTERM; SignalFd::new(sigmask); // or SignalFd::new(&sigmask);

impl From<&Signal> for &SigSet won't work because of trait specialization not being implementated, but there's an even deeper issue. I was trying to get impl AsRef<SigSet> for Signal but realised that it doesn't appear to be possible (given Rust's borrowing model) to return an immutable reference to something allocated in the function that you're returning from. As a simple example, this simply doesn't work for obvious reasons:

impl AsRef<SigSet> for Signal { fn as_ref(&self) -> &SigSet { &self.clone().into() } }

Is there a way to make that work? It seems quite silly that you can't convert &Into<T> into Into<&T> but there's probably some much harder problem here than I'm seeing.

In particular, I'm not really convinced this particular characterbake makes sense:

pub fn signalfd<'a, T: Into<&'a SigSet>>(fd: RawFd, mask: T, flags: SfdFlags) -> Result<RawFd>

In particular, I'm not sure what will actually fulfill the bound Into<&'a SigSet> other than &SigSet -- making it just a waste of characters.

That'd a good point. My question now is why doesn't ppoll take a &SigSet instead of by-value?

Susurrus · 2017-06-06T23:14:30Z

CHANGELOG.md

@@ -16,6 +16,13 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  ([#551](https://github.com/nix-rust/nix/pull/551))
 - Added `nix::pty::{grantpt, posix_openpt, ptsname/ptsname_r, unlockpt}`
  ([#556](https://github.com/nix-rust/nix/pull/556)
+- Added `nix::sys::SigSet::From<Signal>` and refactored some APIs to allow


Since this is a change, it should be put down in the Changed section.

Susurrus · 2017-06-06T23:15:09Z

Getting close now. Some documentation cleanups and a few more functions to convert to use Into<SigSet> and then this should be finished.

Susurrus · 2017-06-28T03:44:45Z

@cyphar You still interested in pushing this over the finish line?

cyphar · 2017-06-28T14:18:33Z

Yes. The past few weeks have been my finals of this semester of university, so I've not been able to touch this. I'll rebase and fix up your comments in the coming days. Thanks.

Susurrus · 2017-07-15T16:08:18Z

@cyphar I'd love to get this in the next release of nix. Will you have time coming up here to rebase this so we can get this merged?

cyphar · 2017-07-16T08:06:24Z

@Susurrus Sorry for the delays, other projects have been getting close to release. I've rebased, but we still haven't figured out how to handle #615 (comment). Should I just remove my Into<&SigSet> changes? And what about ppoll?

The normal libc::sigset_t API is incredibly clunky when initialising SigSets. Since a SigSet semantically is a (mathematical) set of Signals, it makes sense to allow Signals to be promoted to a SigSet. This will also allow us to change our API to take Into<SigSet> rather than SigSet, making this even more idiomatic. let mut sigset = SigSet::empty(); sigset.add(Signal::SIGINT); // versus let sigset: SigSet = Signal::SIGINT.into(); let sigset = SigSet::from(Signal::SIGINT); Signed-off-by: Aleksa Sarai <[email protected]>

In order to make operating on libc::sigset_t even more idiomatic, define several BitOr (and BitOrAssign) implementations. This makes it possible to operate on SigSet much more easily without needing to use SigSet::extend() everywhere. let sigset_a = SigSet::from(Signal::SIGINT); let sigset_b = SigSet::from(Signal::SIGTERM); let mut sigset_c = SigSet::empty(); sigset_c.extend(&sigset_a); sigset_c.extend(&sigset_b); // versus let sigset = Signal::SIGINT | Signal::SIGTERM; Signed-off-by: Aleksa Sarai <[email protected]>

Since we now have From<SigSet> and Into<SigSet>::BitOr, we can make the SigSet API (and also internal usage) far easier to read and more idiomatic. Signed-off-by: Aleksa Sarai <[email protected]>

The old documentation and tests wouldn't compile due to using an old nix API. We probably should enable signalfd building in CI (which is the reason this wasn't caught). Signed-off-by: Aleksa Sarai <[email protected]>

Signed-off-by: Aleksa Sarai <[email protected]>

cyphar · 2017-07-23T06:04:35Z

Rebased again.

Susurrus · 2017-08-01T02:48:35Z

This needs another rebase (be sure to move the CHANGELOG entries into the new unreleased section) and there are some tests failing that need to be fixed.

Susurrus · 2017-10-24T05:31:35Z

@cyphar Can you rebase this and fix the tests that were failing previously?

Susurrus · 2017-12-04T04:29:35Z

There's been no update in almost 5 months, so I'm going to go ahead and close this PR.

cyphar · 2017-12-04T13:13:10Z

I didn't have a chance to rebase it because there was still the question about &SigSet which wasn't resolved, and I didn't see the benefit of rebasing it without resolving that problem first. If you want, I can rebase and create a new PR, but we should figure out what the right solution is for signalfd.

cyphar mentioned this pull request Jun 3, 2017

*Flags don't implement set-like combinators #609

Open

cyphar changed the title ~~implement signal BitOr and SigSet::from~~ implement signal BitOr and sigset! Jun 5, 2017

Susurrus reviewed Jun 5, 2017

View reviewed changes

cyphar changed the title ~~implement signal BitOr and sigset!~~ implement signal BitOr and SigSet::from! Jun 6, 2017

Susurrus suggested changes Jun 6, 2017

View reviewed changes

Susurrus reviewed Jun 6, 2017

View reviewed changes

cyphar added 5 commits July 23, 2017 15:59

*: replace SigSet with Into<SigSet> for input

2145066

Since we now have From<SigSet> and Into<SigSet>::BitOr, we can make the SigSet API (and also internal usage) far easier to read and more idiomatic. Signed-off-by: Aleksa Sarai <[email protected]>

signalfd: fix tests and documentation

7cc2a8f

The old documentation and tests wouldn't compile due to using an old nix API. We probably should enable signalfd building in CI (which is the reason this wasn't caught). Signed-off-by: Aleksa Sarai <[email protected]>

CHANGELOG: update to include changes from #615

ab2f490

Signed-off-by: Aleksa Sarai <[email protected]>

Susurrus closed this Dec 4, 2017

implement signal BitOr and SigSet::from! #615

implement signal BitOr and SigSet::from! #615

Conversation

cyphar commented Jun 3, 2017 • edited Loading

Susurrus commented Jun 3, 2017

Susurrus commented Jun 3, 2017

cyphar commented Jun 3, 2017

cyphar commented Jun 3, 2017

Susurrus commented Jun 4, 2017

cyphar commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Jun 6, 2017

cyphar commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jun 6, 2017

Susurrus commented Jun 28, 2017

cyphar commented Jun 28, 2017

Susurrus commented Jul 15, 2017

cyphar commented Jul 16, 2017

cyphar commented Jul 23, 2017

Susurrus commented Aug 1, 2017

Susurrus commented Oct 24, 2017

Susurrus commented Dec 4, 2017

cyphar commented Dec 4, 2017

cyphar commented Jun 3, 2017 •

edited

Loading