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

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

On Fri, Jan 11, 2013 at 11:54:21PM +0200, Tuomas Jorma Juhani Räsänen wrote:
> This commit changes setup_serve() to report its errors via GError object
> to its caller. It also add resource deallocation code to return points
> to free all local resources correctly. Note that, the previous version
> of the setup_serve() did not free its resources properly on error, but
> just called functions which terminated the process. This was techincally
> a leak, but without any practical significance due to immediate process
> termination. However, now that error handling (process termination) is
> separeted from error reporting, we must deallocate/free all locally
> allocated resources correctly on errors.
> 
> Signed-off-by: Tuomas Jorma Juhani Räsänen <tuomasjjrasanen@...1261...>
> ---
>  nbd-server.c |   73 +++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 54 insertions(+), 19 deletions(-)
> 
> diff --git a/nbd-server.c b/nbd-server.c
> index 87f63fc..be65ebb 100644
> --- a/nbd-server.c
> +++ b/nbd-server.c
> @@ -2392,12 +2392,11 @@ int dosockopts(const int socket, GError **const gerror) {
>   *
>   * @param serve the server we want to connect.
>   **/
> -int setup_serve(SERVER *serve) {
> +int setup_serve(SERVER *const serve, GError **const gerror) {
>  	struct addrinfo hints;
>  	struct addrinfo *ai = NULL;
>  	gchar *port = NULL;
>  	int e;
> -        GError *gerror = NULL;
>  
>          /* Without this, it's possible that socket == 0, even if it's
>           * not initialized at all. And that would be wrong because 0 is
> @@ -2432,10 +2431,12 @@ int setup_serve(SERVER *serve) {
>  	g_free(port);
>  
>  	if(e != 0) {
> -		fprintf(stderr, "getaddrinfo failed: %s\n", gai_strerror(e));
> -		serve->socket = -1;
> -		freeaddrinfo(ai);
> -		exit(EXIT_FAILURE);
> +                g_set_error(gerror, SETUP_ERROR, SETUP_ERROR_GAI,
> +                            "failed to open an export socket: "
> +                            "failed to get address info: %s",
> +                            gai_strerror(e));
> +                freeaddrinfo(ai);
> +                return -1;
>  	}

Same here, obviously.

>  	if(serve->socket_family == AF_UNSPEC)
> @@ -2449,23 +2450,46 @@ int setup_serve(SERVER *serve) {
>  			ai->ai_family = AF_INET6_SDP;
>  	}
>  #endif
> -	if ((serve->socket = socket(ai->ai_family, ai->ai_socktype, ai->ai_protocol)) < 0)
> -		err("socket: %m");
> -
> -	if (dosockopts(serve->socket, &gerror) == -1) {
> -                msg(LOG_ERR, "failed to open export-specific socket: %s",
> -                    gerror->message);
> -                g_clear_error(&gerror);
> -                exit(EXIT_FAILURE);
> +	if ((serve->socket = socket(ai->ai_family, ai->ai_socktype, ai->ai_protocol)) < 0) {
> +                g_set_error(gerror, SETUP_ERROR, SETUP_ERROR_SOCKET,
> +                            "failed to open an export socket: "
> +                            "failed to create a socket: %s",
> +                            strerror(errno));
> +                freeaddrinfo(ai);
> +                return -1;
> +        }
> +
> +	if (dosockopts(serve->socket, gerror) == -1) {
> +                g_prefix_error(gerror, "failed to open an export socket: ");
> +                close(serve->socket);
> +                serve->socket = -1;
> +                freeaddrinfo(ai);
> +                return -1;
>          }
>  
>  	DEBUG("Waiting for connections... bind, ");
>  	e = bind(serve->socket, ai->ai_addr, ai->ai_addrlen);
> -	if (e != 0 && errno != EADDRINUSE)
> -		err("bind: %m");
> +	if (e != 0 && errno != EADDRINUSE) {
> +                g_set_error(gerror, SETUP_ERROR, SETUP_ERROR_BIND,
> +                            "failed to open an export socket: "
> +                            "failed to bind an address to a socket: %s",
> +                            strerror(errno));
> +                close(serve->socket);
> +                serve->socket = -1;
> +                freeaddrinfo(ai);
> +                return -1;
> +        }
>  	DEBUG("listen, ");
> -	if (listen(serve->socket, 1) < 0)
> -		err("listen: %m");
> +	if (listen(serve->socket, 1) < 0) {
> +                g_set_error(gerror, SETUP_ERROR, SETUP_ERROR_BIND,
> +                            "failed to open an export socket: "
> +                            "failed to start listening on a socket: %s",
> +                            strerror(errno));
> +                close(serve->socket);
> +                serve->socket = -1;
> +                freeaddrinfo(ai);
> +                return -1;
> +        }
>  
>  	freeaddrinfo (ai);
>  	if(serve->servename) {
> @@ -2551,7 +2575,18 @@ void setup_servers(GArray *const servers, const gchar *const modernaddr,
>  	int want_modern=0;
>  
>  	for(i=0;i<servers->len;i++) {
> -		want_modern |= setup_serve(&(g_array_index(servers, SERVER, i)));
> +                GError *gerror = NULL;
> +                SERVER *server = &g_array_index(servers, SERVER, i);
> +                int ret;
> +
> +		ret = setup_serve(server, &gerror);
> +                if (ret == -1) {
> +                        msg(LOG_ERR, "failed to setup servers: %s",
> +                            gerror->message);
> +                        g_clear_error(&gerror);
> +                        exit(EXIT_FAILURE);
> +                }
> +                want_modern |= ret;
>  	}
>  	if(want_modern) {
>                  GError *gerror = NULL;
> -- 
> 1.7.10.4
> 
> 
> ------------------------------------------------------------------------------
> Master HTML5, CSS3, ASP.NET, MVC, AJAX, Knockout.js, Web API and
> much more. Get web development skills now with LearnDevNow -
> 350+ hours of step-by-step video tutorials by Microsoft MVPs and experts.
> SALE $99.99 this month only -- learn more at:
> http://p.sf.net/sfu/learnmore_122812
> _______________________________________________
> Nbd-general mailing list
> Nbd-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nbd-general

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