* [Qemu-devel] [PATCH 0/2] net: clean up net/socket.c
@ 2011-12-07 15:01 Stefan Hajnoczi
2011-12-07 15:01 ` [Qemu-devel] [PATCH 1/2] net: expand tabs in net/socket.c Stefan Hajnoczi
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2011-12-07 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: Stefan Hajnoczi
There is no consistent policy on closing the socket file descriptor when
net/socket.c initialization fails. This has been on my TODO list for a while
and I had a few minutes to fix it now.
Stefan Hajnoczi (2):
net: expand tabs in net/socket.c
net: take ownership of fd in socket init functions
net/socket.c | 88 ++++++++++++++++++++++++++++++---------------------------
1 files changed, 46 insertions(+), 42 deletions(-)
--
1.7.7.3
^ permalink raw reply [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 1/2] net: expand tabs in net/socket.c 2011-12-07 15:01 [Qemu-devel] [PATCH 0/2] net: clean up net/socket.c Stefan Hajnoczi @ 2011-12-07 15:01 ` Stefan Hajnoczi 2011-12-08 12:22 ` Zhi Yong Wu 2011-12-07 15:01 ` [Qemu-devel] [PATCH 2/2] net: take ownership of fd in socket init functions Stefan Hajnoczi 2011-12-12 23:34 ` [Qemu-devel] [PATCH 0/2] net: clean up net/socket.c Anthony Liguori 2 siblings, 1 reply; 8+ messages in thread From: Stefan Hajnoczi @ 2011-12-07 15:01 UTC (permalink / raw) To: qemu-devel; +Cc: Stefan Hajnoczi In order to make later patches sane, expand the tab characters and conform to QEMU coding style now. Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- net/socket.c | 79 ++++++++++++++++++++++++++++++---------------------------- 1 files changed, 41 insertions(+), 38 deletions(-) diff --git a/net/socket.c b/net/socket.c index e9ef128..613a7ef 100644 --- a/net/socket.c +++ b/net/socket.c @@ -161,10 +161,11 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr #endif if (!IN_MULTICAST(ntohl(mcastaddr->sin_addr.s_addr))) { - fprintf(stderr, "qemu: error: specified mcastaddr \"%s\" (0x%08x) does not contain a multicast address\n", - inet_ntoa(mcastaddr->sin_addr), + fprintf(stderr, "qemu: error: specified mcastaddr \"%s\" (0x%08x) " + "does not contain a multicast address\n", + inet_ntoa(mcastaddr->sin_addr), (int)ntohl(mcastaddr->sin_addr.s_addr)); - return -1; + return -1; } fd = qemu_socket(PF_INET, SOCK_DGRAM, 0); @@ -177,8 +178,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr ret=setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val)); if (ret < 0) { - perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)"); - goto fail; + perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)"); + goto fail; } ret = bind(fd, (struct sockaddr *)mcastaddr, sizeof(*mcastaddr)); @@ -198,8 +199,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr ret = setsockopt(fd, IPPROTO_IP, IP_ADD_MEMBERSHIP, (const char *)&imr, sizeof(struct ip_mreq)); if (ret < 0) { - perror("setsockopt(IP_ADD_MEMBERSHIP)"); - goto fail; + perror("setsockopt(IP_ADD_MEMBERSHIP)"); + goto fail; } /* Force mcast msgs to loopback (eg. several QEMUs in same host */ @@ -207,8 +208,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr ret=setsockopt(fd, IPPROTO_IP, IP_MULTICAST_LOOP, (const char *)&loop, sizeof(loop)); if (ret < 0) { - perror("setsockopt(SOL_IP, IP_MULTICAST_LOOP)"); - goto fail; + perror("setsockopt(SOL_IP, IP_MULTICAST_LOOP)"); + goto fail; } /* If a bind address is given, only send packets from that address */ @@ -260,37 +261,38 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, */ if (is_connected) { - if (getsockname(fd, (struct sockaddr *) &saddr, &saddr_len) == 0) { - /* must be bound */ - 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; - } - /* 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; - } - /* clone newfd to fd, close newfd */ - dup2(newfd, fd); - close(newfd); - - } else { - fprintf(stderr, "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n", - fd, strerror(errno)); - return NULL; - } + if (getsockname(fd, (struct sockaddr *) &saddr, &saddr_len) == 0) { + /* must be bound */ + 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; + } + /* 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; + } + /* clone newfd to fd, close newfd */ + dup2(newfd, fd); + close(newfd); + + } else { + fprintf(stderr, + "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n", + fd, strerror(errno)); + return NULL; + } } nc = qemu_new_net_client(&net_dgram_socket_info, vlan, NULL, model, name); snprintf(nc->info_str, sizeof(nc->info_str), - "socket: fd=%d (%s mcast=%s:%d)", - fd, is_connected ? "cloned" : "", - inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port)); + "socket: fd=%d (%s mcast=%s:%d)", + fd, is_connected ? "cloned" : "", + inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port)); s = DO_UPCAST(NetSocketState, nc, nc); @@ -349,8 +351,9 @@ static NetSocketState *net_socket_fd_init(VLANState *vlan, if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type, (socklen_t *)&optlen)< 0) { - fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n", fd); - return NULL; + fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n", + fd); + return NULL; } switch(so_type) { case SOCK_DGRAM: @@ -509,7 +512,7 @@ static int net_socket_mcast_init(VLANState *vlan, fd = net_socket_mcast_create(&saddr, param_localaddr); if (fd < 0) - return -1; + return -1; s = net_socket_fd_init(vlan, model, name, fd, 0); if (!s) -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] net: expand tabs in net/socket.c 2011-12-07 15:01 ` [Qemu-devel] [PATCH 1/2] net: expand tabs in net/socket.c Stefan Hajnoczi @ 2011-12-08 12:22 ` Zhi Yong Wu 0 siblings, 0 replies; 8+ messages in thread From: Zhi Yong Wu @ 2011-12-08 12:22 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel On Wed, Dec 7, 2011 at 11:01 PM, Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: > In order to make later patches sane, expand the tab characters and > conform to QEMU coding style now. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > --- > net/socket.c | 79 ++++++++++++++++++++++++++++++---------------------------- > 1 files changed, 41 insertions(+), 38 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index e9ef128..613a7ef 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -161,10 +161,11 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr > #endif > > if (!IN_MULTICAST(ntohl(mcastaddr->sin_addr.s_addr))) { > - fprintf(stderr, "qemu: error: specified mcastaddr \"%s\" (0x%08x) does not contain a multicast address\n", > - inet_ntoa(mcastaddr->sin_addr), > + fprintf(stderr, "qemu: error: specified mcastaddr \"%s\" (0x%08x) " > + "does not contain a multicast address\n", > + inet_ntoa(mcastaddr->sin_addr), > (int)ntohl(mcastaddr->sin_addr.s_addr)); > - return -1; > + return -1; > > } > fd = qemu_socket(PF_INET, SOCK_DGRAM, 0); > @@ -177,8 +178,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr > ret=setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, > (const char *)&val, sizeof(val)); > if (ret < 0) { > - perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)"); > - goto fail; > + perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)"); > + goto fail; > } > > ret = bind(fd, (struct sockaddr *)mcastaddr, sizeof(*mcastaddr)); > @@ -198,8 +199,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr > ret = setsockopt(fd, IPPROTO_IP, IP_ADD_MEMBERSHIP, > (const char *)&imr, sizeof(struct ip_mreq)); > if (ret < 0) { > - perror("setsockopt(IP_ADD_MEMBERSHIP)"); > - goto fail; > + perror("setsockopt(IP_ADD_MEMBERSHIP)"); > + goto fail; > } > > /* Force mcast msgs to loopback (eg. several QEMUs in same host */ > @@ -207,8 +208,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr > ret=setsockopt(fd, IPPROTO_IP, IP_MULTICAST_LOOP, > (const char *)&loop, sizeof(loop)); > if (ret < 0) { > - perror("setsockopt(SOL_IP, IP_MULTICAST_LOOP)"); > - goto fail; > + perror("setsockopt(SOL_IP, IP_MULTICAST_LOOP)"); > + goto fail; > } > > /* If a bind address is given, only send packets from that address */ > @@ -260,37 +261,38 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, > */ > > if (is_connected) { > - if (getsockname(fd, (struct sockaddr *) &saddr, &saddr_len) == 0) { > - /* must be bound */ > - 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; > - } > - /* 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; > - } > - /* clone newfd to fd, close newfd */ > - dup2(newfd, fd); > - close(newfd); > - > - } else { > - fprintf(stderr, "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n", > - fd, strerror(errno)); > - return NULL; > - } > + if (getsockname(fd, (struct sockaddr *) &saddr, &saddr_len) == 0) { > + /* must be bound */ > + 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; > + } > + /* 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; > + } > + /* clone newfd to fd, close newfd */ > + dup2(newfd, fd); > + close(newfd); > + The above white line should be removed. > + } else { > + fprintf(stderr, > + "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n", > + fd, strerror(errno)); > + return NULL; > + } > } > > nc = qemu_new_net_client(&net_dgram_socket_info, vlan, NULL, model, name); > > snprintf(nc->info_str, sizeof(nc->info_str), > - "socket: fd=%d (%s mcast=%s:%d)", > - fd, is_connected ? "cloned" : "", > - inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port)); > + "socket: fd=%d (%s mcast=%s:%d)", > + fd, is_connected ? "cloned" : "", > + inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port)); > > s = DO_UPCAST(NetSocketState, nc, nc); > > @@ -349,8 +351,9 @@ static NetSocketState *net_socket_fd_init(VLANState *vlan, > > if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type, > (socklen_t *)&optlen)< 0) { > - fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n", fd); > - return NULL; > + fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n", > + fd); > + return NULL; > } > switch(so_type) { > case SOCK_DGRAM: > @@ -509,7 +512,7 @@ static int net_socket_mcast_init(VLANState *vlan, > > fd = net_socket_mcast_create(&saddr, param_localaddr); > if (fd < 0) > - return -1; > + return -1; > > s = net_socket_fd_init(vlan, model, name, fd, 0); > if (!s) > -- > 1.7.7.3 > > -- Regards, Zhi Yong Wu ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/2] net: take ownership of fd in socket init functions 2011-12-07 15:01 [Qemu-devel] [PATCH 0/2] net: clean up net/socket.c Stefan Hajnoczi 2011-12-07 15:01 ` [Qemu-devel] [PATCH 1/2] net: expand tabs in net/socket.c Stefan Hajnoczi @ 2011-12-07 15:01 ` Stefan Hajnoczi 2011-12-08 12:29 ` Zhi Yong Wu 2011-12-12 23:34 ` [Qemu-devel] [PATCH 0/2] net: clean up net/socket.c Anthony Liguori 2 siblings, 1 reply; 8+ messages in thread From: Stefan Hajnoczi @ 2011-12-07 15:01 UTC (permalink / raw) To: qemu-devel; +Cc: Stefan Hajnoczi 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 <stefanha@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 { + if (s1) { snprintf(s1->nc.info_str, sizeof(s1->nc.info_str), "socket: connection from %s:%d", inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port)); @@ -549,7 +551,6 @@ int net_init_socket(QemuOpts *opts, } if (!net_socket_fd_init(vlan, "socket", name, fd, 1)) { - close(fd); return -1; } } else if (qemu_opt_get(opts, "listen")) { -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] net: take ownership of fd in socket init functions 2011-12-07 15:01 ` [Qemu-devel] [PATCH 2/2] net: take ownership of fd in socket init functions Stefan Hajnoczi @ 2011-12-08 12:29 ` Zhi Yong Wu 2011-12-08 12:57 ` Stefan Hajnoczi 0 siblings, 1 reply; 8+ messages in thread From: Zhi Yong Wu @ 2011-12-08 12:29 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel On Wed, Dec 7, 2011 at 11:01 PM, Stefan Hajnoczi <stefanha@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 <stefanha@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? > + if (s1) { > snprintf(s1->nc.info_str, sizeof(s1->nc.info_str), > "socket: connection from %s:%d", > inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port)); > @@ -549,7 +551,6 @@ int net_init_socket(QemuOpts *opts, > } > > if (!net_socket_fd_init(vlan, "socket", name, fd, 1)) { > - close(fd); > return -1; > } > } else if (qemu_opt_get(opts, "listen")) { > -- > 1.7.7.3 > > -- Regards, Zhi Yong Wu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] net: take ownership of fd in socket init functions 2011-12-08 12:29 ` Zhi Yong Wu @ 2011-12-08 12:57 ` Stefan Hajnoczi 2011-12-09 12:35 ` Zhi Yong Wu 0 siblings, 1 reply; 8+ messages in thread From: Stefan Hajnoczi @ 2011-12-08 12:57 UTC (permalink / raw) To: Zhi Yong Wu; +Cc: Stefan Hajnoczi, qemu-devel On Thu, Dec 8, 2011 at 12:29 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote: > On Wed, Dec 7, 2011 at 11:01 PM, Stefan Hajnoczi > <stefanha@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 <stefanha@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] net: take ownership of fd in socket init functions 2011-12-08 12:57 ` Stefan Hajnoczi @ 2011-12-09 12:35 ` Zhi Yong Wu 0 siblings, 0 replies; 8+ messages in thread From: Zhi Yong Wu @ 2011-12-09 12:35 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Stefan Hajnoczi, qemu-devel On Thu, Dec 8, 2011 at 8:57 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Thu, Dec 8, 2011 at 12:29 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote: >> On Wed, Dec 7, 2011 at 11:01 PM, Stefan Hajnoczi >> <stefanha@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 <stefanha@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] net: clean up net/socket.c 2011-12-07 15:01 [Qemu-devel] [PATCH 0/2] net: clean up net/socket.c Stefan Hajnoczi 2011-12-07 15:01 ` [Qemu-devel] [PATCH 1/2] net: expand tabs in net/socket.c Stefan Hajnoczi 2011-12-07 15:01 ` [Qemu-devel] [PATCH 2/2] net: take ownership of fd in socket init functions Stefan Hajnoczi @ 2011-12-12 23:34 ` Anthony Liguori 2 siblings, 0 replies; 8+ messages in thread From: Anthony Liguori @ 2011-12-12 23:34 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel On 12/07/2011 09:01 AM, Stefan Hajnoczi wrote: > There is no consistent policy on closing the socket file descriptor when > net/socket.c initialization fails. This has been on my TODO list for a while > and I had a few minutes to fix it now. Applied all. Thanks. Regards, Anthony Liguori > > Stefan Hajnoczi (2): > net: expand tabs in net/socket.c > net: take ownership of fd in socket init functions > > net/socket.c | 88 ++++++++++++++++++++++++++++++--------------------------- > 1 files changed, 46 insertions(+), 42 deletions(-) > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-12-12 23:35 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-07 15:01 [Qemu-devel] [PATCH 0/2] net: clean up net/socket.c Stefan Hajnoczi 2011-12-07 15:01 ` [Qemu-devel] [PATCH 1/2] net: expand tabs in net/socket.c Stefan Hajnoczi 2011-12-08 12:22 ` Zhi Yong Wu 2011-12-07 15:01 ` [Qemu-devel] [PATCH 2/2] net: take ownership of fd in socket init functions Stefan Hajnoczi 2011-12-08 12:29 ` Zhi Yong Wu 2011-12-08 12:57 ` Stefan Hajnoczi 2011-12-09 12:35 ` Zhi Yong Wu 2011-12-12 23:34 ` [Qemu-devel] [PATCH 0/2] net: clean up net/socket.c Anthony Liguori
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).