| Message ID | 1463072585-10566-1-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 12, 2016 at 10:33:05PM +0530, Ashijeet Acharya wrote: > Changed the listen(),connect(),parse_host_port() in net/socket.c with the socket_*()functions in include/qemu/sockets.h. What is the rationale for this change? Please explain why this is necessary or a good idea. Please summarize the address syntax changes in this patch and update the QEMU man page. > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > --- > net/socket.c | 38 +++++++++++++++++++------------------- > 1 file changed, 19 insertions(+), 19 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index 9fa2cd8..b6e2f3e 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -522,10 +522,12 @@ static int net_socket_listen_init(NetClientState *peer, > { > NetClientState *nc; > NetSocketState *s; > - struct sockaddr_in saddr; > + SocketAddress *saddr; > int fd, ret; > + Error *local_error = NULL; > + saddr = g_new0(SocketAddress, 1); > > - if (parse_host_port(&saddr, host_str) < 0) > + if (socket_parse(host_str, &local_error) < 0) > return -1; saddr is leaked. Please check all return code paths and avoid memory leaks. I'm not sure if it makes sense to allocate a new SocketAddress object since it is assigned a different object further down in the patch: saddr = socket_local_address(fd, &local_error);
On Monday 16 May 2016 10:11 PM, Stefan Hajnoczi wrote: > On Thu, May 12, 2016 at 10:33:05PM +0530, Ashijeet Acharya wrote: >> Changed the listen(),connect(),parse_host_port() in net/socket.c with the socket_*()functions in include/qemu/sockets.h. > > What is the rationale for this change? Please explain why this is > necessary or a good idea. This patch consists of basic api conversion since i guess everything will be using QAPI based socket_* functions in the future and the same task was listed on this http://wiki.qemu.org/BiteSizedTasks page too. > > Please summarize the address syntax changes in this patch and update the > QEMU man page. Syntax changes: 1. connect() -> socket_connect() 2. listen() -> socket_listen() 3. parse_host_port() -> socket_parse() 4, delete bind as it is automatically done inside socket_listen. 5. use SocketAddress as data-type of socket variable saddr. > >> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >> --- >> net/socket.c | 38 +++++++++++++++++++------------------- >> 1 file changed, 19 insertions(+), 19 deletions(-) >> >> diff --git a/net/socket.c b/net/socket.c >> index 9fa2cd8..b6e2f3e 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -522,10 +522,12 @@ static int net_socket_listen_init(NetClientState *peer, >> { >> NetClientState *nc; >> NetSocketState *s; >> - struct sockaddr_in saddr; >> + SocketAddress *saddr; >> int fd, ret; >> + Error *local_error = NULL; >> + saddr = g_new0(SocketAddress, 1); >> >> - if (parse_host_port(&saddr, host_str) < 0) >> + if (socket_parse(host_str, &local_error) < 0) >> return -1; > > saddr is leaked. Please check all return code paths and avoid memory > leaks. > > I'm not sure if it makes sense to allocate a new SocketAddress object > since it is assigned a different object further down in the patch: > > saddr = socket_local_address(fd, &local_error); > I have looked into it and hopefully solved the memory leakage problem as you can see in the new patch. Thanks Ashijeet Acharya
On Tue, May 31, 2016 at 03:27:42PM +0530, Ashi wrote: Thanks! Please submit new revisions as separate email threads. Please include the rationale you posted in this message in the commit description so it will be part of the git log. Please check the guidelines on how to submit patches if you have any questions: http://qemu-project.org/Contribute/SubmitAPatch Stefan
diff --git a/net/socket.c b/net/socket.c index 9fa2cd8..b6e2f3e 100644 --- a/net/socket.c +++ b/net/socket.c @@ -522,10 +522,12 @@ static int net_socket_listen_init(NetClientState *peer, { NetClientState *nc; NetSocketState *s; - struct sockaddr_in saddr; + SocketAddress *saddr; int fd, ret; + Error *local_error = NULL; + saddr = g_new0(SocketAddress, 1); - if (parse_host_port(&saddr, host_str) < 0) + if (socket_parse(host_str, &local_error) < 0) return -1; fd = qemu_socket(PF_INET, SOCK_STREAM, 0); @@ -536,14 +538,9 @@ static int net_socket_listen_init(NetClientState *peer, qemu_set_nonblock(fd); socket_set_fast_reuse(fd); + saddr = socket_local_address(fd, &local_error); - ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr)); - if (ret < 0) { - perror("bind"); - closesocket(fd); - return -1; - } - ret = listen(fd, 0); + ret = socket_listen(saddr, &local_error); if (ret < 0) { perror("listen"); closesocket(fd); @@ -557,6 +554,7 @@ static int net_socket_listen_init(NetClientState *peer, s->nc.link_down = true; qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s); + g_free(saddr); return 0; } @@ -567,9 +565,11 @@ static int net_socket_connect_init(NetClientState *peer, { NetSocketState *s; int fd, connected, ret; - struct sockaddr_in saddr; + SocketAddress *saddr; + Error *local_error = NULL; + saddr = g_new0(SocketAddress, 1); - if (parse_host_port(&saddr, host_str) < 0) + if (socket_parse(host_str, &local_error) < 0) return -1; fd = qemu_socket(PF_INET, SOCK_STREAM, 0); @@ -578,10 +578,10 @@ static int net_socket_connect_init(NetClientState *peer, return -1; } qemu_set_nonblock(fd); - + saddr = socket_local_address(fd, &local_error); connected = 0; for(;;) { - ret = connect(fd, (struct sockaddr *)&saddr, sizeof(saddr)); + ret = socket_connect(saddr, &local_error, NULL, NULL); if (ret < 0) { if (errno == EINTR || errno == EWOULDBLOCK) { /* continue */ @@ -602,9 +602,7 @@ static int net_socket_connect_init(NetClientState *peer, s = net_socket_fd_init(peer, model, name, fd, connected); if (!s) return -1; - snprintf(s->nc.info_str, sizeof(s->nc.info_str), - "socket: connect to %s:%d", - inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port)); + g_free(saddr); return 0; } @@ -618,8 +616,9 @@ static int net_socket_mcast_init(NetClientState *peer, int fd; struct sockaddr_in saddr; struct in_addr localaddr, *param_localaddr; + Error *local_error = NULL; - if (parse_host_port(&saddr, host_str) < 0) + if (socket_parse(host_str, &local_error) < 0) return -1; if (localaddr_str != NULL) { @@ -656,12 +655,13 @@ static int net_socket_udp_init(NetClientState *peer, NetSocketState *s; int fd, ret; struct sockaddr_in laddr, raddr; + Error *local_error = NULL; - if (parse_host_port(&laddr, lhost) < 0) { + if (socket_parse(lhost, &local_error) < 0) { return -1; } - if (parse_host_port(&raddr, rhost) < 0) { + if (socket_parse(rhost, &local_error) < 0) { return -1; }
Changed the listen(),connect(),parse_host_port() in net/socket.c with the socket_*()functions in include/qemu/sockets.h. Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> --- net/socket.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)