Add SigSet::union and SigSet::clear.

[fiveop]

No description provided.

[utkarshkukreti]

The man page for sigsetops says:

NAME
     sigaddset, sigdelset, sigemptyset, sigfillset, sigismember -- manipulate signal sets

...

RETURN VALUES
     The sigismember() function returns 1 if the signal is a member of the set, 0 otherwise.  The other functions
     return 0.

ERRORS
     Currently, no errors are detected.

So should these functions really need to return a Result<()> instead of nothing?

[fiveop]

That holds for the whole module. I will change that in a sepearte commit (I can add it to this PR though).

http://linux.die.net/man/3/sigaddset disagrees. Which platform are you using?

[utkarshkukreti]

Ah, ignore my comment. It seems that the OSX versions don't ever return an error according to the man pages on my system (OSX 10.11).

[fiveop]

Any other critique? Or can we merge this?

[fiveop]

r? @kamalmarhubi

[kamalmarhubi]

I'm surprised this is a loop! I don't know if we want what looks like 1-2 bitwise operations to actually end up being ~50-100. My feeling is that at this level, small code should have small cost.

[fiveop]

You would think! But even as a user of libc we should be unaware of the implementation details of sigaddset etc.

[kamalmarhubi]

Oh interesting re errors. If / when we move to a Signal enum, we'd should never have invalid signals and so these could return plain values.

I have one comment about unexpected number of operations from union(). Given it's not in the standard library I lean towards not adding it, or figuring out how to do it in a bitwise operation or two.

[kamalmarhubi]

@fiveop I assume these came out of your need. Could you say a tiny bit about where union() is more useful than add()?

[fiveop]

Regarding the errors that is correct. I wonder what OS X does, if you pass it an invalid signal value.

These kind of functions are exactly what I am looking for in nix. I don't want them in my application or library.

Regarding the usecase. Suppose I have a base mask that I always want to block. Then I have depending on the situation various sets that I want to block in addition. As an example look at: https://github.com/sbcl/sbcl/blob/master/src/runtime/interrupt.c#L233. Note that they implemented their own function sigcopyset as a memcpy. This is sort of safe, because you do not need to know anything about the internal structure of a sigset. However, a bitwise or for union() might be wrong (It probably never is though). Suppose for example, that the system uses 0s for blocked and 1 for non-blocked signals (for whatever reason).

[kamalmarhubi]

This makes sense to me. The only thing I wonder is if we should change union() to insert() to match the name in the bitflags crate: https://doc.rust-lang.org/bitflags/src/bitflags/src/lib.rs.html#350

We can always attempt to optimize it if we think it matters, and add some fancier testing.

[fiveop]

I changed the name. union is a better name for a function taking two sets and producing a third one. insert implies the change of an existing set. That's good.

[kamalmarhubi]

Not seeing the change. Could you also update the commit message and PR title when you do. Then r=me.

[utkarshkukreti]

In other Set/Map APIs in Rust insert refers to inserting 1 item into the Set or the Map. It's probably fine for bitflags because you can also insert just one bitflag or a set of bitflags but an insert method that only accepts a SigSet seems unidiomatic.

[fiveop]

How about going with add_set?

[utkarshkukreti]

I believe the idiomatic way to merge a HashSet into another HashSet is a.extend(b). Maybe we could call it extend?

[fiveop]

In that case we can go all the way and implement Iterator, IntoIterator and Extend for SigSet. I'll prepare corresponding change.

[utkarshkukreti]

I would wait for @kamalmarhubi's views.

[kamalmarhubi]

In that case we can go all the way and implement Iterator, IntoIterator and Extend for SigSet. I'll prepare corresponding change.

I was actually just coming here to suggest this. Being able to iterate over the SigSet seems like a nice plus, too. I'd include FromIterator while you're at it.

The traits are all in core, so #[nostd] users of nix would still have access. @utkarshkukreti does it make sense to you to do it?

[utkarshkukreti]

Sounds good @kamalmarhubi!

[fiveop]

Implementing Extend is currently a problem. Extending a SigSet from an arbitrary iterator of SigNums, which is an alias for some integer type, could cause an error. It becomes safe, when we have changed SigNum to be an enumeration. I would do that next in a seperate PR. Then I can get back to implement Extend, etc.

I have changed the name to extend for now.

[kamalmarhubi]

This seems good to me since the name will be change. The return type would drop the Result but it's not much churn.

@homu r+ 8698588

Re enums, I started today on a libc_enum macro so we could easily set up things like SigNum as a enums with values from the libc crate. Sadly I'm running into some trouble. This is definitely a change I'd like to see. Personally I'd also like to change SigNum to Signal at that point since it's no longer semantically a number, but that's for the other issue PR.

[homu]

⌛ Testing commit 8698588 with merge 3ed76e4...

[homu]

☀️ Test successful - status