From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57478) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RYdXs-0007gI-AZ for qemu-devel@nongnu.org; Thu, 08 Dec 2011 07:57:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RYdXp-0005vJ-TS for qemu-devel@nongnu.org; Thu, 08 Dec 2011 07:57:52 -0500 Received: from mail-ww0-f53.google.com ([74.125.82.53]:37318) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RYdXp-0005vC-Np for qemu-devel@nongnu.org; Thu, 08 Dec 2011 07:57:49 -0500 Received: by wgbds1 with SMTP id ds1so3162560wgb.10 for ; Thu, 08 Dec 2011 04:57:48 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1323270109-24265-1-git-send-email-stefanha@linux.vnet.ibm.com> <1323270109-24265-3-git-send-email-stefanha@linux.vnet.ibm.com> Date: Thu, 8 Dec 2011 12:57:48 +0000 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/2] net: take ownership of fd in socket init functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhi Yong Wu Cc: Stefan Hajnoczi , qemu-devel@nongnu.org On Thu, Dec 8, 2011 at 12:29 PM, Zhi Yong Wu wrote: > On Wed, Dec 7, 2011 at 11:01 PM, Stefan Hajnoczi > wrote: >> Today net/socket.c has no consistent policy for closing the socket file >> descriptor when initialization fails. =A0This 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 >> --- >> =A0net/socket.c | =A0 17 +++++++++-------- >> =A01 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(VL= ANState *vlan, >> =A0 =A0 =A0 =A0 =A0 =A0 if (saddr.sin_addr.s_addr =3D=3D 0) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fprintf(stderr, "qemu: error: init_dgram= : fd=3D%d unbound, " >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "cannot setup multicast = dst addr\n", fd); >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return NULL; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err; >> =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 =A0 =A0 /* clone dgram socket */ >> =A0 =A0 =A0 =A0 =A0 =A0 newfd =3D net_socket_mcast_create(&saddr, NULL); >> =A0 =A0 =A0 =A0 =A0 =A0 if (newfd < 0) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* error already reported by net_socket_= mcast_create() */ >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0close(fd); >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return NULL; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err; >> =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 =A0 =A0 /* clone newfd to fd, close newfd */ >> =A0 =A0 =A0 =A0 =A0 =A0 dup2(newfd, fd); >> @@ -283,7 +282,7 @@ static NetSocketState *net_socket_fd_init_dgram(VLAN= State *vlan, >> =A0 =A0 =A0 =A0 =A0 =A0 fprintf(stderr, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "qemu: error: init_dgram: fd=3D%= d failed getsockname(): %s\n", >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fd, strerror(errno)); >> - =A0 =A0 =A0 =A0 =A0 =A0return NULL; >> + =A0 =A0 =A0 =A0 =A0 =A0goto err; >> =A0 =A0 =A0 =A0 } >> =A0 =A0 } >> >> @@ -304,6 +303,10 @@ static NetSocketState *net_socket_fd_init_dgram(VLA= NState *vlan, >> =A0 =A0 if (is_connected) s->dgram_dst=3Dsaddr; >> >> =A0 =A0 return s; >> + >> +err: >> + =A0 =A0closesocket(fd); >> + =A0 =A0return NULL; >> =A0} >> >> =A0static void net_socket_connect(void *opaque) >> @@ -353,6 +356,7 @@ static NetSocketState *net_socket_fd_init(VLANState = *vlan, >> =A0 =A0 =A0 =A0 (socklen_t *)&optlen)< 0) { >> =A0 =A0 =A0 =A0 fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd= =3D%d failed\n", >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fd); >> + =A0 =A0 =A0 =A0closesocket(fd); >> =A0 =A0 =A0 =A0 return NULL; >> =A0 =A0 } >> =A0 =A0 switch(so_type) { >> @@ -386,9 +390,7 @@ static void net_socket_accept(void *opaque) >> =A0 =A0 =A0 =A0 } >> =A0 =A0 } >> =A0 =A0 s1 =3D net_socket_fd_init(s->vlan, s->model, s->name, fd, 1); >> - =A0 =A0if (!s1) { >> - =A0 =A0 =A0 =A0closesocket(fd); >> - =A0 =A0} 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