On Thu, Dec 8, 2011 at 12:29 PM, Zhi Yong Wu <zwu.ker...@gmail.com> wrote:
> On Wed, Dec 7, 2011 at 11:01 PM, Stefan Hajnoczi
> <stefa...@linux.vnet.ibm.com> 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 <stefa...@linux.vnet.ibm.com>
>> ---
>> 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
