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

[Zhi Yong Wu]

On Thu, Dec 8, 2011 at 8:57 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Thu, Dec 8, 2011 at 12:29 PM, Zhi Yong Wu <address@hidden> wrote:
>> 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?
>
> The point of the patch is to introduce consistent error behavior -
> net_socket_fd_init() will close the socket on error so we no longer
> have to do that.  If you look at net_socket_accept() there is nothing
> else to do on failure it was possible to just remove the if (!s1)
> check.
You are right, thanks.
>
> Stefan



-- 
Regards,

Zhi Yong Wu