-
Notifications
You must be signed in to change notification settings - Fork 47
Add signal functions (modify, pending) #351
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
Conversation
92398d0 to
8f66707
Compare
lib/lualinux.c
Outdated
| sigaddset(&newmask, signum); | ||
|
|
||
| lunatik_try(L, sigprocmask, cmd, &newmask, NULL); | ||
| lua_pushboolean(L, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why pushboolean here?
lib/lualinux.c
Outdated
| SIGSTATE_ALLOWED, | ||
| }; | ||
|
|
||
| static const char *const sigstate_opts[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it shouldn't be static, right?
lib/lualinux.c
Outdated
| [SIGSTATE_BLOCKED] = "blocked", | ||
| [SIGSTATE_PENDING] = "pending", | ||
| [SIGSTATE_ALLOWED] = "allowed", | ||
| NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| NULL | |
| NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, we don't need this NULL here, right?
| }; | ||
|
|
||
| int signum = luaL_checkinteger(L, 1); | ||
| enum sigstate_cmd cmd = (enum sigstate_cmd)luaL_checkoption(L, 2, "blocked", sigstate_opts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should have a default = blocked here.. but I will leave this decision to you ;-).. what would be the better ergonomics for this API..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel like its better than having pending at the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thing is.. we don't need to have any default.. but it's alright to me.. =)
lneto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's looking good to me.. just small comments..
No description provided.