Re: [Qemu-devel] [PATCH 2/2] net: take ownership of fd in socket init functions

[Zhi Yong Wu]

On Wed, Dec 7, 2011 at 11:01 PM, Stefan Hajnoczi
<address@hidden> wrote:
> Today net/socket.c has no consistent policy for closing the socket file
> descriptor when initialization fails.  This means we leak the file
> descriptor in some cases or we could also try to close it twice.
>
> Make error paths consistent by taking ownership of the file descriptor
> and closing it on error.
>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  net/socket.c |   17 +++++++++--------
>  1 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index 613a7ef..f999c26 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -266,14 +266,13 @@ static NetSocketState 
> *net_socket_fd_init_dgram(VLANState *vlan,
>             if (saddr.sin_addr.s_addr == 0) {
>                 fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, "
>                         "cannot setup multicast dst addr\n", fd);
> -                return NULL;
> +                goto err;
>             }
>             /* clone dgram socket */
>             newfd = net_socket_mcast_create(&saddr, NULL);
>             if (newfd < 0) {
>                 /* error already reported by net_socket_mcast_create() */
> -                close(fd);
> -                return NULL;
> +                goto err;
>             }
>             /* clone newfd to fd, close newfd */
>             dup2(newfd, fd);
> @@ -283,7 +282,7 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState 
> *vlan,
>             fprintf(stderr,
>                     "qemu: error: init_dgram: fd=%d failed getsockname(): 
> %s\n",
>                     fd, strerror(errno));
> -            return NULL;
> +            goto err;
>         }
>     }
>
> @@ -304,6 +303,10 @@ static NetSocketState 
> *net_socket_fd_init_dgram(VLANState *vlan,
>     if (is_connected) s->dgram_dst=saddr;
>
>     return s;
> +
> +err:
> +    closesocket(fd);
> +    return NULL;
>  }
>
>  static void net_socket_connect(void *opaque)
> @@ -353,6 +356,7 @@ static NetSocketState *net_socket_fd_init(VLANState *vlan,
>         (socklen_t *)&optlen)< 0) {
>         fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
>                 fd);
> +        closesocket(fd);
>         return NULL;
>     }
>     switch(so_type) {
> @@ -386,9 +390,7 @@ static void net_socket_accept(void *opaque)
>         }
>     }
>     s1 = net_socket_fd_init(s->vlan, s->model, s->name, fd, 1);
> -    if (!s1) {
> -        closesocket(fd);
> -    } else {
Why is it not handled when s1 is NULL?
> +    if (s1) {
>         snprintf(s1->nc.info_str, sizeof(s1->nc.info_str),
>                  "socket: connection from %s:%d",
>                  inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
> @@ -549,7 +551,6 @@ int net_init_socket(QemuOpts *opts,
>         }
>
>         if (!net_socket_fd_init(vlan, "socket", name, fd, 1)) {
> -            close(fd);
>             return -1;
>         }
>     } else if (qemu_opt_get(opts, "listen")) {
> --
> 1.7.7.3
>
>



-- 
Regards,

Zhi Yong Wu