From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55838) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9cRP-0003Ty-HD for qemu-devel@nongnu.org; Sun, 05 Jun 2016 14:06:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b9cRK-0001T6-78 for qemu-devel@nongnu.org; Sun, 05 Jun 2016 14:06:26 -0400 Received: from mail-pa0-x241.google.com ([2607:f8b0:400e:c03::241]:35593) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9cRJ-0001Sv-TC for qemu-devel@nongnu.org; Sun, 05 Jun 2016 14:06:22 -0400 Received: by mail-pa0-x241.google.com with SMTP id gp3so10060730pac.2 for ; Sun, 05 Jun 2016 11:06:21 -0700 (PDT) References: <20160516164123.GC15256@stefanha-x1.localdomain> <1464686839-9547-1-git-send-email-ashijeetacharya@gmail.com> From: Ashijeet Acharya Message-ID: <57546A16.9090306@gmail.com> Date: Sun, 5 Jun 2016 23:36:14 +0530 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Modify net/socket.c to use socket_* functions from include/qemu/sockets.h List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , jasowang@redhat.com Cc: stefanha@gmail.com, qemu-devel@nongnu.org On Tuesday 31 May 2016 08:31 PM, Paolo Bonzini wrote: > > > On 31/05/2016 11:27, Ashijeet Acharya wrote: >> 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 >> --- >> 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; >> >> - if (parse_host_port(&saddr, host_str) < 0) >> + if (socket_parse(host_str, &local_error) < 0) > > This leaks the return address. > > The result of socket_parse should be stored in saddr. > > Also, the right comparison is "!= NULL", not "< 0". > > Finally, you're not printing the error (with error_report_err). > Solved this....although I think the comparison will be "== NULL". >> 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); > > This is incorrect too. You're adding a call to getsockname that wasn't > in the original code. You're also ignoring possible failures from > socket_local_address. > >> - 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"); > > You should use error_report_err instead of perror, since that's how > socket_listen returns errors. You are also leaking saddr. > Done. I am not sure how saddr is getting leaked here. >> 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); > > Use qapi_free_SocketAddress instead of g_free, because SocketAddress > includes pointers to other data structures that have to be freed. Done. > >> 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; >> >> - if (parse_host_port(&saddr, host_str) < 0) >> + if (socket_parse(host_str, &local_error) < 0) > > Same as above. > >> 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); > > Same as above. You probably haven't tested this patch well enough, > because otherwise you would have noticed the problem here. > >> 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)); > > Here you should have created a new function socket_address_to_string in > util/qemu-sockets.c. The function should return a new string > corresponding to the address. The address can be IPv4/IPv6 (then > printing is done via inet_ntop), a Unix socket or a file descriptor; you > have to handle all three cases. > > The return value of the function can be used together with snprintf to > form s->nc.info_str. I created the new function in util/qemu-sockets.c, will it be right to use sprintf() instead of inet_ntop() like the way its done in inet_ntoa()? > >> + 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; > > Same problem as above. In addition, saddr is being passed uninitialized > to net_socket_mcast_create. Here net_socket_mcast_create() takes argument of the type struct sockaddr_in, but if i change saddr to the type struct SocketAddress to use it with socket_parse() the whole thing becomes a mess. How do i tackle this?? Please bear with me...I am a newbie and this is my first patch. > >> 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; >> } > > Same problem as above. In addition, laddr and raddr are being used > uninitialized. At least in the case of raddr, the compiler should have > noticed this and issued a warning. > > Thanks, > > Paolo > Thanks Ashijeet