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 <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).
>>
> 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
