Re: [Qemu-devel] [PATCH] Modify net/socket.c to use socket_* functions from include/qemu/sockets.h

[Ashijeet Acharya <ashijeetacharya@gmail.com>]

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