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 <ashijeetacha...@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
