| Message ID | 1464686839-9547-1-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <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; > > - 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). > 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. > 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. > 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. > + 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. > 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
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 <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; >> >> - 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
On 05/06/2016 20:06, Ashijeet Acharya wrote: > > > 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 <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; >>> >>> - 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". Right. >>> - 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. It was allocated in socket_parse, and you're returning without freeing it. >>> @@ -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()? I'm not sure I understand... To print an address with address family AF_INET6 you need inet_ntop. >>> @@ -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?? You can leave net_socket_mcast_init and net_socket_mcast_create aside for now. Thanks, Paolo
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) return -1; fd = qemu_socket(PF_INET, SOCK_STREAM, 0); @@ -536,14 +538,9 @@ static int net_socket_listen_init(NetClientState *peer,
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(-) 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; - 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; }