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

[Paolo Bonzini <pbonzini@redhat.com>]

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