On Thu, Dec 8, 2011 at 8:57 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> 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.
You are right, thanks.
>
> Stefan
--
Regards,
Zhi Yong Wu
