Re: [Nbd] [PATCH 1/1] nbd-server: read conf on SIGHUP and add new exports

[Wouter Verhelst <w@...112...>]

On Sun, Mar 10, 2013 at 10:01:45PM +0200, Tuomas Räsänen wrote:
> This patch makes the root server process re-read its configuration files
> and add new exports dynamically when SIGHUP is sent. The rehash feature
> is non-destructive: it does not modify any existing exports, it only
> lets the root server process to add new exports. However, it might be a
> good idea to implement destructive counterparts (export deletion and
> modification) too at some point.
> 
> Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@...1261...>
> ---
>  man/nbd-server.1.in.sgml |    7 +++
>  nbd-server.c             |  124 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 131 insertions(+)
> 
> diff --git a/man/nbd-server.1.in.sgml b/man/nbd-server.1.in.sgml
> index 7a5832e..a6adce4 100644
> --- a/man/nbd-server.1.in.sgml
> +++ b/man/nbd-server.1.in.sgml
> @@ -95,6 +95,13 @@ manpage.1: manpage.sgml
>      export, the use of this option is deprecated. It is preferred to
>      make use of a configuration file instead, the format of which is
>      defined in nbd-server(5).</para>
> +
> +   <para>While nbd-server is running, new exports can be added by
> +   re-writing configuration files and then sending SIGHUP to
> +   nbd-server. SIGHUP causes nbd-server to re-read its configuration
> +   files and to start serving all new exports which were not served
> +   earlier. Reconfiguration does not modify any existing export, it only
> +   appends new ones.</para>
>    </refsect1>
>    <refsect1>
>      <title>OPTIONS</title>
> diff --git a/nbd-server.c b/nbd-server.c
> index d49874d..27c4600 100644
> --- a/nbd-server.c
> +++ b/nbd-server.c
> @@ -177,6 +177,11 @@ char default_authname[] = SYSCONFDIR "/nbd-server/allow"; /**< default name of a
>  #define NEG_OLD		(1 << 1)
>  #define NEG_MODERN	(1 << 2)
>  
> +static volatile sig_atomic_t is_sighup_caught; /**< Flag set by SIGHUP
> +                                                    handler to mark a
> +                                                    reconfiguration
> +                                                    request */
> +
>  int modernsock=-1;	  /**< Socket for the modern handler. Not used
>  			       if a client was only specified on the
>  			       command line; only port used if
> @@ -1117,6 +1122,19 @@ void sigterm_handler(int s) {
>  }
>  
>  /**
> + * Handle SIGHUP by setting atomically a flag which will be evaluated in
> + * the main loop of the root server process. This allows us to separate
> + * the signal catching from th actual task triggered by SIGHUP and hence
> + * processing in the interrupt context is kept as minimial as possible.
> + *
> + * @param s the signal we're handling (must be SIGHUP, or something
> + * is severely wrong).
> + **/
> +static void sighup_handler(const int s __attribute__ ((unused))) {

__attribute__ flags are GCC-specific and not necessarily portable to
other compilers.

I'll change that to G_GNUC_UNUSED, a macro that is defined by glib, and
which (I believe) is already used in other places.

[...]
> +        for (i = 0; i < new_servers->len; ++i) {
> +                SERVER new_server = g_array_index(new_servers, SERVER, i);
> +
> +                if (new_server.servename
> +                    && -1 == get_index_by_servename(new_server.servename,
> +                                                    servers)) {
> +                        if (setup_serve(&new_server, gerror) == -1)
> +                                goto out;
> +                        if (append_serve(&new_server, servers) == -1)
> +                                goto out;

It might make sense to produce an error message here, but I suppose
that's not crucial (append_serve and/or setup_serve probably already do
so?)

Other than that, looks good. Applied, thanks.

-- 
Copyshops should do vouchers. So that next time some bureaucracy requires you
to mail a form in triplicate, you can mail it just once, add a voucher, and
save on postage.