Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link
  • 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.

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.

  • 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.

  • 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.

Added NULL check
Source Link
Niall C.
  • 859
  • 1
  • 7
  • 19
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);
}
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 */
 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);
}
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);
}
added 388 characters in body
Source Link
Niall C.
  • 859
  • 1
  • 7
  • 19

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.

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.

Same advice about changing the sigprocmask()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.

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.

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.

Same advice about changing the sigprocmask() call as for block_signal() above.

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.

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.

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.

added 391 characters in body
Source Link
Niall C.
  • 859
  • 1
  • 7
  • 19
Loading
Source Link
Niall C.
  • 859
  • 1
  • 7
  • 19
Loading
lang-c

AltStyle によって変換されたページ (->オリジナル) /