static struct sigaction *
handle_signal(int sig,
int flags,
void (*handler)())
{
Two things here:
The signal handler function should have the signature void handler(int signum); [man page for sigaction] . Your compiler should — at the least — be warning you about assigning a function pointer of a different type further down.
The function returns a struct sigaction * containing the original disposition for the signal. None of the other functions shown accept a struct sigaction * as an argument, so it's not clear what you're doing with this information.
If you need this information elsewhere, I suggest passing in a struct sigaction * as an additional argument. The POSIX sigaction() API allows you to pass NULL as the third parameter, so this would leave it to the caller to decide whether or not they wanted to provide a pointer to a real struct sigaction. This also ties into the use of statically allocated structures which I address next.
static struct sigaction action;
static struct sigaction o_action;
You declare all of your <signal.h> structures with static storage class, so there is only one instance of each of them in the program's memory space. If the program using this API is multithreaded, and this API is called from multiple locations, you can overwrite the variables on one thread while they're being populated from another, causing you lots of confusion. Since the following code immediately zeros out their memory, you can remove the static from the declarations, so that they get allocated on the stack. That way if the function is called from multiple threads, they each get their own copy and don't conflict with each other.
/* init the sigaction structs */
memset(&action, 0, sizeof(action));
memset(&o_action, 0, sizeof(o_action));
/* setup our signal handler function to call when it fires */
action.sa_handler = handler;
/* initialize the signal mask */
sigemptyset(&action.sa_mask);
/* use the user passed in flags setting othersize init them */
if (flags != 0)
action.sa_flags = flags;
else
action.sa_flags = 0;
The else clause is taken if flags has the value 0, so action.sa_flags = 0 is equivalent to action.sa_flags = flags, so this whole if-statement could be collapsed to:
action.sa_flags = flags;
if (sigaction(sig, &action, &o_action) != 0)
syserr();
return(&o_action);
}
Taken altogether, this function could be rewritten:
static struct sigaction *
handle_signal(int sig,
int flags,
void (*handler)(int),
struct sigaction *o_action)
{
struct sigaction action;
memset(&action, 0, sizeof(action));
/* Note: can't use sizeof(o_action) below because o_action is now a pointer */
if(o_action != NULL)
memset(o_action, 0, sizeof(action));
/* setup our signal handler function to call when it fires */
action.sa_handler = handler;
/* initialize the signal mask */
sigemptyset(&action.sa_mask);
action.sa_flags = flags;
if (sigaction(sig, &action, &o_action) != 0)
syserr();
return(o_action);
}
If you're using the original signal disposition to restore things to their defaults later on, you could potentially drop o_action altogether and install SIG_DFL as the handler for that signal. Change the function to return void, only have the original three parameters, change the sigaction call to sigaction(sig, &action, &o_action) and remove the return statement at the end.
static struct sigaction *
ignore_signal(int sig)
{
/* Code elided */
}
There's a lot of common code between this and handle_signal above; I'd suggest replacing the body of this function with a call to that function instead:
static struct sigaction *
ignore_signal(int sig)
{
return handle_signal(sig, 0, SIG_IGN);
}
int
block_signal(int sig)
{
static sigset_t mask;
static sigset_t orig_mask;
int ret;
sigemptyset(&mask);
sigemptyset(&orig_mask);
sigaddset(&mask, sig);
ret = sigprocmask(SIG_BLOCK, &mask, &orig_mask);
return(ret);
}
You're not doing anything with the value of orig_mask after the return from sigprocmask(). This system call allows you to pass NULL [man page for sigprocmask], so I suggest you change it to:
ret = sigprocmask(SIG_BLOCK, &mask, NULL);
int
unblock_signal(int sig)
{
static sigset_t mask;
static sigset_t orig_mask;
int ret;
sigemptyset(&mask);
sigemptyset(&orig_mask);
sigaddset(&mask, sig);
ret = sigprocmask(SIG_UNBLOCK, &mask, &orig_mask);
return(ret);
}
Same advice about changing the sigprocmask() call as for block_signal() above. I also note that in the get_signal_mask() function below, you're calling your syserr() function if sigprocmask() fails. It seems to me that if you're exiting the program in that situation, you should also be doing it here where you're actually changing the mask, not merely querying its value.
Additionally, block_signal() and unblock_signal() differ only in the first argument to sigprocmask(), so I'd suggest refactoring the code in both functions into a single helper function that accepts the SIG_BLOCK or SIG_UNBLOCK argument.
sigset_t *
get_signal_mask()
{
static sigset_t orig_mask;
sigemptyset(&orig_mask);
/* null in set arg means return current mask */
if (sigprocmask(SIG_SETMASK, NULL, &orig_mask) < 0)
syserr();
return(&orig_mask);
}
Again, you're returning a pointer to a statically allocated instance of sigset_t. On multithreaded programs, this can cause problems as already explained. I suggest changing it either to accept a sigset_t * argument, so that the caller must provide the instance to populate (this is the more C-like way of doing it):
sigset_t *
get_signal_mask(sigset_t *orig_mask)
{
sigemptyset(orig_mask);
/* null in set arg means return current mask */
if (sigprocmask(SIG_SETMASK, NULL, orig_mask) < 0)
syserr();
return(orig_mask);
}
OR have the function return the sigset_t by value and remove the static from the declaration of orig_mask (this is the more C++-like way of doing it):
sigset_t
get_signal_mask()
{
sigset_t orig_mask;
sigemptyset(&orig_mask);
/* null in set arg means return current mask */
if (sigprocmask(SIG_SETMASK, NULL, &orig_mask) < 0)
syserr();
return(orig_mask);
}
Both methods are valid in both languages, so whichever you prefer.
