From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49048) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RYd6F-0007sV-MQ for qemu-devel@nongnu.org; Thu, 08 Dec 2011 07:29:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RYd6E-0001Sr-KM for qemu-devel@nongnu.org; Thu, 08 Dec 2011 07:29:19 -0500 Received: from mail-vx0-f173.google.com ([209.85.220.173]:47459) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RYd6E-0001Sl-HU for qemu-devel@nongnu.org; Thu, 08 Dec 2011 07:29:18 -0500 Received: by vcbfl11 with SMTP id fl11so1422190vcb.4 for ; Thu, 08 Dec 2011 04:29:18 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1323270109-24265-3-git-send-email-stefanha@linux.vnet.ibm.com> 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 20:29:18 +0800 Message-ID: From: Zhi Yong Wu 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: Stefan Hajnoczi Cc: qemu-devel@nongnu.org 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(VLA= NState *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 d= st 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_m= cast_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(VLANS= tate *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(VLAN= State *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? > + =A0 =A0if (s1) { > =A0 =A0 =A0 =A0 snprintf(s1->nc.info_str, sizeof(s1->nc.info_str), > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"socket: connection from %s:%d", > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0inet_ntoa(saddr.sin_addr), ntohs(saddr= .sin_port)); > @@ -549,7 +551,6 @@ int net_init_socket(QemuOpts *opts, > =A0 =A0 =A0 =A0 } > > =A0 =A0 =A0 =A0 if (!net_socket_fd_init(vlan, "socket", name, fd, 1)) { > - =A0 =A0 =A0 =A0 =A0 =A0close(fd); > =A0 =A0 =A0 =A0 =A0 =A0 return -1; > =A0 =A0 =A0 =A0 } > =A0 =A0 } else if (qemu_opt_get(opts, "listen")) { > -- > 1.7.7.3 > > --=20 Regards, Zhi Yong Wu