Re: [Nbd] [PATCH 2/5] nbd-server: fix open_modern() to report errors

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

On Fri, Jan 11, 2013 at 11:54:02PM +0200, Tuomas Jorma Juhani Räsänen wrote:
> This commit is part of the refactoring work on internal error
> management. The goal of the work is to separate error reporting from
> error handling to make it possible to call functions from different code
> paths with varying needs for error handling.
> 
> Another goal is to unify the error management throughout the code base.
> 
> Signed-off-by: Tuomas Jorma Juhani Räsänen <tuomasjjrasanen@...1261...>
> ---
>  nbd-server.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/nbd-server.c b/nbd-server.c
> index abb647c..87f63fc 100644
> --- a/nbd-server.c
> +++ b/nbd-server.c
> @@ -640,6 +640,10 @@ typedef enum {
>          SETUP_ERROR_SO_LINGER,               /**< Failed to set SO_LINGER to a socket */
>          SETUP_ERROR_SO_REUSEADDR,            /**< Failed to set SO_REUSEADDR to a socket */
>          SETUP_ERROR_SO_KEEPALIVE,            /**< Failed to set SO_KEEPALIVE to a socket */
> +        SETUP_ERROR_GAI,                     /**< Failed to get address info */
> +        SETUP_ERROR_SOCKET,                  /**< Failed to create a socket */
> +        SETUP_ERROR_BIND,                    /**< Failed to bind an address to socket */
> +        SETUP_ERROR_LISTEN,                  /**< Failed to start listening on a socket */
>  } SETUP_ERRORS;
>  
>  /**
> @@ -2471,12 +2475,12 @@ int setup_serve(SERVER *serve) {
>  	}
>  }
>  
> -void open_modern(const gchar *const addr, const gchar *const port) {
> +int open_modern(const gchar *const addr, const gchar *const port,
> +                GError **const gerror) {
>  	struct addrinfo hints;
>  	struct addrinfo* ai = NULL;
>  	struct sock_flags;
>  	int e;
> -        GError *gerror = NULL;
>  
>  	memset(&hints, '\0', sizeof(hints));
>  	hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG;
> @@ -2485,28 +2489,56 @@ void open_modern(const gchar *const addr, const gchar *const port) {
>  	hints.ai_protocol = IPPROTO_TCP;
>  	e = getaddrinfo(addr, port ? port : NBD_DEFAULT_PORT, &hints, &ai);
>  	if(e != 0) {
> -		fprintf(stderr, "getaddrinfo failed: %s\n", gai_strerror(e));
> -		exit(EXIT_FAILURE);
> +                g_set_error(gerror, SETUP_ERROR, SETUP_ERROR_GAI,
> +                            "failed to open a modern socket: "
> +                            "failed to get address info: %s",
> +                            gai_strerror(e));
> +                freeaddrinfo(ai);
> +                return -1;
>  	}
> +
>  	if((modernsock = socket(ai->ai_family, ai->ai_socktype, ai->ai_protocol))<0) {
> -		err("socket: %m");
> +                g_set_error(gerror, SETUP_ERROR, SETUP_ERROR_SOCKET,
> +                            "failed to open a modern socket: "
> +                            "failed to create a socket: %s",
> +                            strerror(errno));
> +                freeaddrinfo(ai);
> +                return -1;
>  	}
>  
> -	if (dosockopts(modernsock, &gerror) == -1) {
> -                msg(LOG_ERR, "failed to open service-wide socket: %s",
> -                    gerror->message);
> -                g_clear_error(&gerror);
> -                exit(EXIT_FAILURE);
> +	if (dosockopts(modernsock, gerror) == -1) {
> +                g_prefix_error(gerror, "failed to open a modern socket: ");
> +                close(modernsock);
> +                modernsock = -1;
> +                freeaddrinfo(ai);
> +                return -1;
>          }
>  
>  	if(bind(modernsock, ai->ai_addr, ai->ai_addrlen)) {
> -		err("bind: %m");
> +                g_set_error(gerror, SETUP_ERROR, SETUP_ERROR_BIND,
> +                            "failed to open a modern socket: "
> +                            "failed to bind an address to a socket: %s",
> +                            strerror(errno));
> +                close(modernsock);
> +                modernsock = -1;
> +                freeaddrinfo(ai);
> +                return -1;
>  	}
> +
>  	if(listen(modernsock, 10) <0) {
> -		err("listen: %m");
> +                g_set_error(gerror, SETUP_ERROR, SETUP_ERROR_BIND,
> +                            "failed to open a modern socket: "
> +                            "failed to start listening on a socket: %s",
> +                            strerror(errno));
> +                close(modernsock);
> +                modernsock = -1;
> +                freeaddrinfo(ai);
> +                return -1;
>  	}
>  	freeaddrinfo(ai);
> +
> +        return 0;
>  }

This having to repeatedly close or free or initialize a particular value
creates a lot of clutter, which makes it harder to follow what's really
going on. It's also a possible source of future bugs; when we add
another variable, or change how we need to deinitialize it, or something
similar, we'd need to ensure that each and every one of these if blocks
is updated; if we miss one, we introduce a possible leak.

Instead, I'd prefer if you could use goto here. This is about the only
situation where goto is a good idea, but then it's a really good idea:

  int retval = 0;
  if(foo) {
	g_set_error(blabla);
	retval = -1;
	goto err_state_1;
  }
  [...]
  if(bar) {
	g_set_error(blabla);
	retval = -1;
	goto err_state_2;
  }
  [...]
err_state_1:
  g_free(baz);
err_state_2:
  g_free(quux);
  return retval;

This keeps the deinitialization code together (so updating how to
deinitialize something is done in one central location). It also keeps
the if blocks much shorter, making the code easier to read.

(obviously the goto labels would use some descriptive name instead, but
you got that, right? ;-)

>  
>  /**
> @@ -2522,7 +2554,13 @@ void setup_servers(GArray *const servers, const gchar *const modernaddr,
>  		want_modern |= setup_serve(&(g_array_index(servers, SERVER, i)));
>  	}
>  	if(want_modern) {
> -		open_modern(modernaddr, modernport);
> +                GError *gerror = NULL;
> +                if (open_modern(modernaddr, modernport, &gerror) == -1) {
> +                        msg(LOG_ERR, "failed to setup servers: %s",
> +                            gerror->message);
> +                        g_clear_error(&gerror);
> +                        exit(EXIT_FAILURE);
> +                }
>  	}
>  	children=g_hash_table_new_full(g_int_hash, g_int_equal, NULL, destroy_pid_t);

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