[pacman-dev] [PATCH 2/6] dload: move (un)masking of signals to separate functions

[Dan McGee]

On Thu, Aug 18, 2011 at 8:09 PM, Dave Reisner <d at falconindy.com> wrote:
> Signed-off-by: Dave Reisner <dreisner at archlinux.org>
> ---
> mask_signal doesn't get inlined here, but unmask does.
>
>  lib/libalpm/dload.c |   43 +++++++++++++++++++++++++------------------
>  1 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 16b2206..f8c10fa 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -67,7 +67,6 @@ static char *get_fullpath(const char *path, const char *filename,
>  }
>
>  #define check_stop() if(dload_interrupted) { ret = -1; goto cleanup; }
> -enum sighandlers { OLD = 0, NEW = 1 };
>
>  static int dload_interrupted;
>  static void inthandler(int UNUSED signum)
> @@ -234,6 +233,25 @@ static int curl_set_handle_opts(struct dload_payload *payload,
>        return 0;
>  }
>
> +/* set new sighandler, and return the original */
> +static struct sigaction mask_signal(int signal, void (*handler)(int))
> +{
> +       struct sigaction newaction, origaction;
> +
> +       newaction.sa_handler = handler;
> +       sigemptyset(&newaction.sa_mask);
> +       newaction.sa_flags = 0;
> +
> +       sigaction(signal, NULL, &origaction);
> +       sigaction(signal, &newaction, NULL);
> +
> +       return origaction;
> +}
> +
> +static void unmask_signal(int signal, struct sigaction sa)
> +{
> +       sigaction(signal, &sa, NULL);
> +}
>
>  static int curl_download_internal(struct dload_payload *payload,
>                const char *localpath, char **final_file)
> @@ -247,7 +265,7 @@ static int curl_download_internal(struct dload_payload *payload,
>        char error_buffer[CURL_ERROR_SIZE] = { 0 };
>        long timecond, remote_time = -1;
>        double remote_size, bytes_dl;
> -       struct sigaction sig_pipe[2], sig_int[2];
> +       struct sigaction sig_pipe, sig_int;
>        /* shortcut to our handle within the payload */
>        alpm_handle_t *handle = payload->handle;
>        handle->pm_errno = 0;
> @@ -294,20 +312,9 @@ static int curl_download_internal(struct dload_payload *payload,
>                goto cleanup;
>        }
>
> -       /* ignore any SIGPIPE signals- these may occur if our FTP socket dies or
> -        * something along those lines. Store the old signal handler first. */
> -       sig_pipe[NEW].sa_handler = SIG_IGN;
> -       sigemptyset(&sig_pipe[NEW].sa_mask);
> -       sig_pipe[NEW].sa_flags = 0;
> -       sigaction(SIGPIPE, NULL, &sig_pipe[OLD]);
> -       sigaction(SIGPIPE, &sig_pipe[NEW], NULL);
> -
> -       dload_interrupted = 0;
> -       sig_int[NEW].sa_handler = &inthandler;
> -       sigemptyset(&sig_int[NEW].sa_mask);
> -       sig_int[NEW].sa_flags = 0;
> -       sigaction(SIGINT, NULL, &sig_int[OLD]);
> -       sigaction(SIGINT, &sig_int[NEW], NULL);
> +       /* ignore SIGPIPE and SIGINT signals while we're transferring */
> +       sig_pipe = mask_signal(SIGPIPE, SIG_IGN);
> +       sig_int = mask_signal(SIGINT, &inthandler);
Now that we don't have the enum, it would be clearer if these
variables were named "orig_sigpipe" or something- it took me a while
to figure out why the second var wasn't needed here anymore.

>
>        /* Progress 0 - initialize */
>        prevprogress = 0;
> @@ -414,8 +421,8 @@ cleanup:
>        FREE(destfile);
>
>        /* restore the old signal handlers */
> -       sigaction(SIGINT, &sig_int[OLD], NULL);
> -       sigaction(SIGPIPE, &sig_pipe[OLD], NULL);
> +       unmask_signal(SIGINT, sig_int);
> +       unmask_signal(SIGPIPE, sig_pipe);

Should we be doing this sooner? e.g. as soon as we are done
downloading? Or does this location make sense so that we are atomic in
all our download stuff before tripping the original handler right
before we return from the function. A question more than knowing the
right answer here.

>        /* if we were interrupted, trip the old handler */
>        if(dload_interrupted) {
>                raise(SIGINT);
> --
> 1.7.6
>
>
>