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

[Stefan Hajnoczi]

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.

Stefan