Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Account for the different signal handler types. #241

Closed
wants to merge 2 commits into from
Closed

Account for the different signal handler types. #241

wants to merge 2 commits into from

Conversation

fiveop
Copy link
Contributor

@fiveop fiveop commented Jan 20, 2016

This commit would be an API breaking change: nix::sys::signals::sigaction get's a different signature.

It currently leaves the responsibility of passing the SA_SIGINFO flag if necessary to the user.

This might solve part of #49.

@fiveop fiveop mentioned this pull request Jan 20, 2016
@fiveop
Copy link
Contributor Author

fiveop commented Jan 22, 2016

Thinking more about this, the constructor function new is now unsafe. If you pass in a extern fn(SigNum, *mut siginfo, *mut libc::c_void) you also have to pass flags with SA_SIGINFO and for extern fn(SigNum) the flag must not be set.

I see two solutions: Either add/remove those flags everytime (just in case) change the return value to a Result and signal an error, if the flags are not appropriatly set. Both go against the idea of zero cost abstractions. However, we cannot provide a safe interface without them.

The third option is to mark the method unsafe and describe in a comment why. This way the caller knows what he has to do, to ensure safety.

@kamalmarhubi
Copy link
Member

I'm not following the reasoning behind not wanting to force the correct setting of SA_SIGINFO based on the enum variant. This seems like a perfect place for nix to help the user do the right thing.

@fiveop
Copy link
Contributor Author

fiveop commented Feb 1, 2016

My problem was (and still is) that it is no longer a zero cost abstraction. But all our error handling code already adds additional costs, so I opted to push a variant that sets or unsets the flag as necessary.

If someone creates so many SigAction structs, that a few extra
instructions per object creation create a performance problem,
we could still provide an unsafe variant, that let's the user take
care of the flag.
};
s.sa_flags = match handler {
SigHandler::SigAction(_) => flags | SA_SIGINFO,
_ => flags - SA_SIGINFO,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think flags & !SA_SIGINFO is nicer when removing a single flag. As it is, I had to go and double check that we actually had a bitflags type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the minus sign, which represents set difference. And that is exactly what we want to express and achieve here. In addition it is more concise.
I do not think that preventing confusion is a valid argument since the type is stated in the parameter list and (now) by convention we use bitflags for such types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a very reasonable position!

@kamalmarhubi
Copy link
Member

Regarding "zero cost", I think your commit comment captures what that means. It's ok to put in a few extra instructions for the infrequent thing, but the actual calling of the signal handlers will have no cost above what you'd expect in C. This differs from, say, Go which handles signals by passing them to channels, or Python, where the signal handlers are Python functions and so there's some per-signal overhead there.

@fiveop
Copy link
Contributor Author

fiveop commented Feb 4, 2016

Unless there are further concerns I would like to merge this, in particular, because I want to use it (and apparently others too).

@kamalmarhubi
Copy link
Member

Sounds good to me!

@fiveop
Copy link
Contributor Author

fiveop commented Feb 4, 2016

Merged as 645c48f...645e37d.

@fiveop fiveop closed this Feb 4, 2016
@kamalmarhubi kamalmarhubi mentioned this pull request Feb 15, 2016
@fiveop fiveop deleted the sigaction branch February 28, 2016 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants