Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Please feel free to comment on the accuracy/validity of the following wrapper source for processing signals using the new POSIX sigaction API. If you feel I'm doing anything wrong or potentially dangerous, chime in.

Note: syserr is a custom function not shown to exit gracefully.


#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <string.h>
#include <errno.h>

static struct sigaction *
handle_signal(int sig,
              int flags,
              void (*handler)())
{
static struct sigaction action;
static struct sigaction o_action;

/* 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;

if (sigaction(sig, &action, &o_action) != 0)
   syserr();

return(&o_action);

}

static struct sigaction *
ignore_signal(int sig)
{
static struct sigaction action;
static struct sigaction o_action;

/* init the sigaction structs */
memset(&action, 0, sizeof(action));
memset(&o_action, 0, sizeof(o_action));

/* setup to ignore this signal */
action.sa_handler = SIG_IGN;

/* initialize the signal mask */
sigemptyset(&action.sa_mask);

action.sa_flags = 0;

if (sigaction(sig, &action, &o_action) != 0)
   syserr();

return(&o_action);

}

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);
}

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);

}

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);

}
share|improve this question
1  
Post rolled back. The new code should be appended to this post, not edited directly into the original code. If you'd prefer further review on the new code, post a new question. –  Jamal Apr 12 at 21:59

1 Answer 1

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.

share|improve this answer
    
A superb answer! +1 –  syb0rg Apr 12 at 15:21
    
appreciate your detailed response. –  user3053087 Apr 14 at 13:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.