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

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

[Ashijeet Acharya]

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;
+    saddr = g_new0(SocketAddress, 1);
 
-    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,
     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;
+    saddr = g_new0(SocketAddress, 1);
 
-    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;
     }
 
-- 
1.9.1

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

On Thu, May 12, 2016 at 10:33:05PM +0530, Ashijeet Acharya wrote:
> Changed the listen(),connect(),parse_host_port() in net/socket.c with the socket_*()functions in include/qemu/sockets.h.

What is the rationale for this change?  Please explain why this is
necessary or a good idea.

Please summarize the address syntax changes in this patch and update the
QEMU man page.

> 
> 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;
> +    saddr = g_new0(SocketAddress, 1);
>  
> -    if (parse_host_port(&saddr, host_str) < 0)
> +    if (socket_parse(host_str, &local_error) < 0)
>          return -1;

saddr is leaked.  Please check all return code paths and avoid memory
leaks.

I'm not sure if it makes sense to allocate a new SocketAddress object
since it is assigned a different object further down in the patch:

saddr = socket_local_address(fd, &local_error);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Ashijeet Acharya]

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)
         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);
 
-    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;
     }
 
-- 
1.9.1

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

[Ashi]

On Monday 16 May 2016 10:11 PM, Stefan Hajnoczi wrote:
> On Thu, May 12, 2016 at 10:33:05PM +0530, Ashijeet Acharya wrote:
>> Changed the listen(),connect(),parse_host_port() in net/socket.c with the socket_*()functions in include/qemu/sockets.h.
>
> What is the rationale for this change?  Please explain why this is
> necessary or a good idea.

This patch consists of basic api conversion since i guess everything 
will be using QAPI based socket_* functions in the future and the same 
task was listed on this http://wiki.qemu.org/BiteSizedTasks page too.

>
> Please summarize the address syntax changes in this patch and update the
> QEMU man page.

Syntax changes:

1. connect() -> socket_connect()
2. listen() -> socket_listen()
3. parse_host_port() -> socket_parse()
4, delete bind as it is automatically done inside socket_listen.
5. use SocketAddress as data-type of socket variable saddr.

>
>>
>> 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;
>> +    saddr = g_new0(SocketAddress, 1);
>>
>> -    if (parse_host_port(&saddr, host_str) < 0)
>> +    if (socket_parse(host_str, &local_error) < 0)
>>           return -1;
>
> saddr is leaked.  Please check all return code paths and avoid memory
> leaks.
>
> I'm not sure if it makes sense to allocate a new SocketAddress object
> since it is assigned a different object further down in the patch:
>
> saddr = socket_local_address(fd, &local_error);
>
I have looked into it and hopefully solved the memory leakage problem as 
you can see in the new patch.


Thanks
Ashijeet Acharya

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

[Paolo Bonzini]

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

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

[Ashijeet Acharya]

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

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

[Paolo Bonzini]

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On Tue, May 31, 2016 at 03:27:42PM +0530, Ashi wrote:
Thanks!

Please submit new revisions as separate email threads.

Please include the rationale you posted in this message in the commit
description so it will be part of the git log.

Please check the guidelines on how to submit patches if you have any
questions:
http://qemu-project.org/Contribute/SubmitAPatch

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH] Change net/socket.c to use socket_*() functions

[Ashijeet Acharya]

Use socket_*() functions from include/qemu/sockets.h instead of listen()/bind()/  connect()/parse_host_port(). socket_*() fucntions are QAPI based and this patch   performs this api conversion since everything will be using QAPI based sockets in the future. Also add a helper function socket_address_to_string() in              util/qemu-sockets.c which returns the string representation of socket address. The task was listed on http://wiki.qemu.org/BiteSizedTasks page.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 include/qemu/sockets.h | 16 ++++++++++++++-
 net/socket.c           | 56 +++++++++++++++++++++++++-------------------------
 util/qemu-sockets.c    | 25 ++++++++++++++++++++++
 3 files changed, 68 insertions(+), 29 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 1bd9218..3a1a887 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -110,4 +110,18 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
 void qapi_copy_SocketAddress(SocketAddress **p_dest,
                              SocketAddress *src);
 
-#endif /* QEMU_SOCKET_H */
+/**
+ * socket_address_to_string:
+ * @addr: the socket address struct
+ * @errp: pointer to uninitialized error object
+ *
+ * Get the string representation of the socket
+ * address. A pointer to the char array containing
+ * string format will be returned, the caller is
+ * required to release the returned value when no
+ * longer required with g_free.
+ *
+ * Returns: the socket address in string format, or NULL on error
+ */
+char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
+#endif /* QEMU_SOCKET_H */
\ No newline at end of file
diff --git a/net/socket.c b/net/socket.c
index 333fb9e..b4d1576 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -489,41 +489,30 @@ static int net_socket_listen_init(NetClientState *peer,
 {
     NetClientState *nc;
     NetSocketState *s;
-    struct sockaddr_in saddr;
-    int fd, ret;
+    SocketAddress *saddr;
+    int ret;
+    Error *local_error = NULL;
 
-    if (parse_host_port(&saddr, host_str) < 0)
-        return -1;
-
-    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (fd < 0) {
-        perror("socket");
+    saddr = socket_parse(host_str, &local_error);
+    if (saddr == NULL) {
+        error_report_err(local_error);
         return -1;
     }
-    qemu_set_nonblock(fd);
 
-    socket_set_fast_reuse(fd);
-
-    ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
+    ret = socket_listen(saddr, &local_error);
     if (ret < 0) {
-        perror("bind");
-        closesocket(fd);
-        return -1;
-    }
-    ret = listen(fd, 0);
-    if (ret < 0) {
-        perror("listen");
-        closesocket(fd);
+        error_report_err(local_error);
         return -1;
     }
 
     nc = qemu_new_net_client(&net_socket_info, peer, model, name);
     s = DO_UPCAST(NetSocketState, nc, nc);
     s->fd = -1;
-    s->listen_fd = fd;
+    s->listen_fd = ret;
     s->nc.link_down = true;
 
     qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
+    qapi_free_SocketAddress(saddr);
     return 0;
 }
 
@@ -534,10 +523,15 @@ static int net_socket_connect_init(NetClientState *peer,
 {
     NetSocketState *s;
     int fd, connected, ret;
-    struct sockaddr_in saddr;
+    char *addr_str;
+    SocketAddress *saddr;
+    Error *local_error = NULL;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    saddr = socket_parse(host_str, &local_error);
+    if (saddr == NULL) {
+        error_report_err(local_error);
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
@@ -545,10 +539,9 @@ static int net_socket_connect_init(NetClientState *peer,
         return -1;
     }
     qemu_set_nonblock(fd);
-
     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 */
@@ -557,7 +550,7 @@ static int net_socket_connect_init(NetClientState *peer,
                        errno == EINVAL) {
                 break;
             } else {
-                perror("connect");
+                error_report_err(local_error);
                 closesocket(fd);
                 return -1;
             }
@@ -569,9 +562,16 @@ static int net_socket_connect_init(NetClientState *peer,
     s = net_socket_fd_init(peer, model, name, fd, connected);
     if (!s)
         return -1;
+
+    addr_str = socket_address_to_string(saddr, &local_error);
+    if (addr_str == NULL)
+        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));
+             "socket: connect to %s:%s",
+             addr_str, saddr->u.inet.data->port);
+    qapi_free_SocketAddress(saddr);
+    g_free(addr_str);
     return 0;
 }
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 0d6cd1f..191e0e0 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1151,3 +1151,28 @@ void qapi_copy_SocketAddress(SocketAddress **p_dest,
     qmp_input_visitor_cleanup(qiv);
     qobject_decref(obj);
 }
+
+char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
+{
+    char *buf;
+
+    switch (addr->type) {
+    case SOCKET_ADDRESS_KIND_INET:
+        buf = g_strdup(addr->u.inet.data->host);
+        break;
+
+    case SOCKET_ADDRESS_KIND_UNIX:
+        buf = g_strdup(addr->u.q_unix.data->path);
+        break;
+
+    case SOCKET_ADDRESS_KIND_FD:
+        buf = g_strdup(addr->u.fd.data->str);
+        break;
+
+    default:
+        error_setg(errp, "socket family %d unsupported",
+                   addr->type);
+        return NULL;
+    }
+    return buf;
+}
-- 
2.6.2

Re: [Qemu-devel] [PATCH] Change net/socket.c to use socket_*() functions

[Paolo Bonzini]

Thanks, much better!  Just one thing:

On 16/06/2016 12:20, Ashijeet Acharya wrote:
> +    case SOCKET_ADDRESS_KIND_INET:
> +        buf = g_strdup(addr->u.inet.data->host);

Please include the port too here.  Also, if the host contains a colon
(which you can check with strchr), please print it as [HOST]:PORT
instead of HOST:PORT.  This will help with IPv6.

Thanks,

Paolo

> +        break;
> +
> +    case SOCKET_ADDRESS_KIND_UNIX:
> +        buf = g_strdup(addr->u.q_unix.data->path);
> +        break;
> +
> +    case SOCKET_ADDRESS_KIND_FD:
> +        buf = g_strdup(addr->u.fd.data->str);
> +        break;
> +
> +    default:
> +        error_setg(errp, "socket family %d unsupported",
> +                   addr->type);
> +        return NULL;

[Qemu-devel] [PATCH v2] Change net/socket.c to use socket_*() functions

[Ashijeet Acharya]

Use socket_*() functions from include/qemu/sockets.h instead of listen()/bind()/  connect()/parse_host_port(). socket_*() fucntions are QAPI based and this patch   performs this api conversion since everything will be using QAPI based sockets in the future. Also add a helper function socket_address_to_string() in              util/qemu-sockets.c which returns the string representation of socket address. Thetask was listed on http://wiki.qemu.org/BiteSizedTasks page.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 include/qemu/sockets.h | 16 ++++++++++++++-
 net/socket.c           | 55 +++++++++++++++++++++++++-------------------------
 util/qemu-sockets.c    | 36 +++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 29 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 1bd9218..3a1a887 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -110,4 +110,18 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
 void qapi_copy_SocketAddress(SocketAddress **p_dest,
                              SocketAddress *src);
 
-#endif /* QEMU_SOCKET_H */
+/**
+ * socket_address_to_string:
+ * @addr: the socket address struct
+ * @errp: pointer to uninitialized error object
+ *
+ * Get the string representation of the socket
+ * address. A pointer to the char array containing
+ * string format will be returned, the caller is
+ * required to release the returned value when no
+ * longer required with g_free.
+ *
+ * Returns: the socket address in string format, or NULL on error
+ */
+char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
+#endif /* QEMU_SOCKET_H */
\ No newline at end of file
diff --git a/net/socket.c b/net/socket.c
index 333fb9e..ae6f921 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -489,41 +489,30 @@ static int net_socket_listen_init(NetClientState *peer,
 {
     NetClientState *nc;
     NetSocketState *s;
-    struct sockaddr_in saddr;
-    int fd, ret;
+    SocketAddress *saddr;
+    int ret;
+    Error *local_error = NULL;
 
-    if (parse_host_port(&saddr, host_str) < 0)
-        return -1;
-
-    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (fd < 0) {
-        perror("socket");
+    saddr = socket_parse(host_str, &local_error);
+    if (saddr == NULL) {
+        error_report_err(local_error);
         return -1;
     }
-    qemu_set_nonblock(fd);
 
-    socket_set_fast_reuse(fd);
-
-    ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
+    ret = socket_listen(saddr, &local_error);
     if (ret < 0) {
-        perror("bind");
-        closesocket(fd);
-        return -1;
-    }
-    ret = listen(fd, 0);
-    if (ret < 0) {
-        perror("listen");
-        closesocket(fd);
+        error_report_err(local_error);
         return -1;
     }
 
     nc = qemu_new_net_client(&net_socket_info, peer, model, name);
     s = DO_UPCAST(NetSocketState, nc, nc);
     s->fd = -1;
-    s->listen_fd = fd;
+    s->listen_fd = ret;
     s->nc.link_down = true;
 
     qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
+    qapi_free_SocketAddress(saddr);
     return 0;
 }
 
@@ -534,10 +523,15 @@ static int net_socket_connect_init(NetClientState *peer,
 {
     NetSocketState *s;
     int fd, connected, ret;
-    struct sockaddr_in saddr;
+    char *addr_str;
+    SocketAddress *saddr;
+    Error *local_error = NULL;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    saddr = socket_parse(host_str, &local_error);
+    if (saddr == NULL) {
+        error_report_err(local_error);
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
@@ -545,10 +539,9 @@ static int net_socket_connect_init(NetClientState *peer,
         return -1;
     }
     qemu_set_nonblock(fd);
-
     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 */
@@ -557,7 +550,7 @@ static int net_socket_connect_init(NetClientState *peer,
                        errno == EINVAL) {
                 break;
             } else {
-                perror("connect");
+                error_report_err(local_error);
                 closesocket(fd);
                 return -1;
             }
@@ -569,9 +562,15 @@ static int net_socket_connect_init(NetClientState *peer,
     s = net_socket_fd_init(peer, model, name, fd, connected);
     if (!s)
         return -1;
+
+    addr_str = socket_address_to_string(saddr, &local_error);
+    if (addr_str == NULL)
+        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));
+             "socket: connect to %s", addr_str);
+    qapi_free_SocketAddress(saddr);
+    g_free(addr_str);
     return 0;
 }
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 0d6cd1f..771b7db 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1151,3 +1151,39 @@ void qapi_copy_SocketAddress(SocketAddress **p_dest,
     qmp_input_visitor_cleanup(qiv);
     qobject_decref(obj);
 }
+
+char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
+{
+    char *buf;
+    InetSocketAddress *inet;
+    char host_port[INET6_ADDRSTRLEN + 5 + 4];
+
+    switch (addr->type) {
+    case SOCKET_ADDRESS_KIND_INET:
+        inet = addr->u.inet.data;
+        if (strchr(inet->host, ':') == NULL) {
+            snprintf(host_port, sizeof(host_port), "%s:%s", inet->host,
+                    inet->port);
+            buf = g_strdup(host_port);
+        } else {
+            snprintf(host_port, sizeof(host_port), "[%s]:%s", inet->host,
+                    inet->port);
+            buf = g_strdup(host_port);
+        }
+        break;
+
+    case SOCKET_ADDRESS_KIND_UNIX:
+        buf = g_strdup(addr->u.q_unix.data->path);
+        break;
+
+    case SOCKET_ADDRESS_KIND_FD:
+        buf = g_strdup(addr->u.fd.data->str);
+        break;
+
+    default:
+        error_setg(errp, "socket family %d unsupported",
+                   addr->type);
+        return NULL;
+    }
+    return buf;
+}
-- 
2.6.2

Re: [Qemu-devel] [PATCH v2] Change net/socket.c to use socket_*() functions

[Paolo Bonzini]

On 18/06/2016 09:54, Ashijeet Acharya wrote:
> Use socket_*() functions from include/qemu/sockets.h instead of
> listen()/bind()/ connect()/parse_host_port(). socket_*() fucntions are
> QAPI based and this patch performs this api conversion since everything
> will be using QAPI based sockets in the future. Also add a helper
> function socket_address_to_string() in util/qemu-sockets.c which returns
> the string representation of socket address. Thetask was listed on
> http://wiki.qemu.org/BiteSizedTasks page.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Jason, are you going to take this through the net tree?

Thanks,

Paolo

> ---
>  include/qemu/sockets.h | 16 ++++++++++++++-
>  net/socket.c           | 55 +++++++++++++++++++++++++-------------------------
>  util/qemu-sockets.c    | 36 +++++++++++++++++++++++++++++++++
>  3 files changed, 78 insertions(+), 29 deletions(-)
> 
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 1bd9218..3a1a887 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -110,4 +110,18 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
>  void qapi_copy_SocketAddress(SocketAddress **p_dest,
>                               SocketAddress *src);
>  
> -#endif /* QEMU_SOCKET_H */
> +/**
> + * socket_address_to_string:
> + * @addr: the socket address struct
> + * @errp: pointer to uninitialized error object
> + *
> + * Get the string representation of the socket
> + * address. A pointer to the char array containing
> + * string format will be returned, the caller is
> + * required to release the returned value when no
> + * longer required with g_free.
> + *
> + * Returns: the socket address in string format, or NULL on error
> + */
> +char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
> +#endif /* QEMU_SOCKET_H */
> \ No newline at end of file
> diff --git a/net/socket.c b/net/socket.c
> index 333fb9e..ae6f921 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -489,41 +489,30 @@ static int net_socket_listen_init(NetClientState *peer,
>  {
>      NetClientState *nc;
>      NetSocketState *s;
> -    struct sockaddr_in saddr;
> -    int fd, ret;
> +    SocketAddress *saddr;
> +    int ret;
> +    Error *local_error = NULL;
>  
> -    if (parse_host_port(&saddr, host_str) < 0)
> -        return -1;
> -
> -    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> -    if (fd < 0) {
> -        perror("socket");
> +    saddr = socket_parse(host_str, &local_error);
> +    if (saddr == NULL) {
> +        error_report_err(local_error);
>          return -1;
>      }
> -    qemu_set_nonblock(fd);
>  
> -    socket_set_fast_reuse(fd);
> -
> -    ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
> +    ret = socket_listen(saddr, &local_error);
>      if (ret < 0) {
> -        perror("bind");
> -        closesocket(fd);
> -        return -1;
> -    }
> -    ret = listen(fd, 0);
> -    if (ret < 0) {
> -        perror("listen");
> -        closesocket(fd);
> +        error_report_err(local_error);
>          return -1;
>      }
>  
>      nc = qemu_new_net_client(&net_socket_info, peer, model, name);
>      s = DO_UPCAST(NetSocketState, nc, nc);
>      s->fd = -1;
> -    s->listen_fd = fd;
> +    s->listen_fd = ret;
>      s->nc.link_down = true;
>  
>      qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
> +    qapi_free_SocketAddress(saddr);
>      return 0;
>  }
>  
> @@ -534,10 +523,15 @@ static int net_socket_connect_init(NetClientState *peer,
>  {
>      NetSocketState *s;
>      int fd, connected, ret;
> -    struct sockaddr_in saddr;
> +    char *addr_str;
> +    SocketAddress *saddr;
> +    Error *local_error = NULL;
>  
> -    if (parse_host_port(&saddr, host_str) < 0)
> +    saddr = socket_parse(host_str, &local_error);
> +    if (saddr == NULL) {
> +        error_report_err(local_error);
>          return -1;
> +    }
>  
>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>      if (fd < 0) {
> @@ -545,10 +539,9 @@ static int net_socket_connect_init(NetClientState *peer,
>          return -1;
>      }
>      qemu_set_nonblock(fd);
> -
>      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 */
> @@ -557,7 +550,7 @@ static int net_socket_connect_init(NetClientState *peer,
>                         errno == EINVAL) {
>                  break;
>              } else {
> -                perror("connect");
> +                error_report_err(local_error);
>                  closesocket(fd);
>                  return -1;
>              }
> @@ -569,9 +562,15 @@ static int net_socket_connect_init(NetClientState *peer,
>      s = net_socket_fd_init(peer, model, name, fd, connected);
>      if (!s)
>          return -1;
> +
> +    addr_str = socket_address_to_string(saddr, &local_error);
> +    if (addr_str == NULL)
> +        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));
> +             "socket: connect to %s", addr_str);
> +    qapi_free_SocketAddress(saddr);
> +    g_free(addr_str);
>      return 0;
>  }
>  
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 0d6cd1f..771b7db 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1151,3 +1151,39 @@ void qapi_copy_SocketAddress(SocketAddress **p_dest,
>      qmp_input_visitor_cleanup(qiv);
>      qobject_decref(obj);
>  }
> +
> +char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
> +{
> +    char *buf;
> +    InetSocketAddress *inet;
> +    char host_port[INET6_ADDRSTRLEN + 5 + 4];
> +
> +    switch (addr->type) {
> +    case SOCKET_ADDRESS_KIND_INET:
> +        inet = addr->u.inet.data;
> +        if (strchr(inet->host, ':') == NULL) {
> +            snprintf(host_port, sizeof(host_port), "%s:%s", inet->host,
> +                    inet->port);
> +            buf = g_strdup(host_port);
> +        } else {
> +            snprintf(host_port, sizeof(host_port), "[%s]:%s", inet->host,
> +                    inet->port);
> +            buf = g_strdup(host_port);
> +        }
> +        break;
> +
> +    case SOCKET_ADDRESS_KIND_UNIX:
> +        buf = g_strdup(addr->u.q_unix.data->path);
> +        break;
> +
> +    case SOCKET_ADDRESS_KIND_FD:
> +        buf = g_strdup(addr->u.fd.data->str);
> +        break;
> +
> +    default:
> +        error_setg(errp, "socket family %d unsupported",
> +                   addr->type);
> +        return NULL;
> +    }
> +    return buf;
> +}
> 

Re: [Qemu-devel] [PATCH v2] Change net/socket.c to use socket_*() functions

[Peter Maydell]

On 20 June 2016 at 15:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 18/06/2016 09:54, Ashijeet Acharya wrote:
>> Use socket_*() functions from include/qemu/sockets.h instead of
>> listen()/bind()/ connect()/parse_host_port(). socket_*() fucntions are
>> QAPI based and this patch performs this api conversion since everything
>> will be using QAPI based sockets in the future. Also add a helper
>> function socket_address_to_string() in util/qemu-sockets.c which returns
>> the string representation of socket address. Thetask was listed on
>> http://wiki.qemu.org/BiteSizedTasks page.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Jason, are you going to take this through the net tree?

Can you fix up the long lines/space issues in the commit
message if you do, please?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] Change net/socket.c to use socket_*() functions

[Jason Wang]

On 2016年06月20日 23:09, Peter Maydell wrote:
> On 20 June 2016 at 15:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 18/06/2016 09:54, Ashijeet Acharya wrote:
>>> Use socket_*() functions from include/qemu/sockets.h instead of
>>> listen()/bind()/ connect()/parse_host_port(). socket_*() fucntions are
>>> QAPI based and this patch performs this api conversion since everything
>>> will be using QAPI based sockets in the future. Also add a helper
>>> function socket_address_to_string() in util/qemu-sockets.c which returns
>>> the string representation of socket address. Thetask was listed on
>>> http://wiki.qemu.org/BiteSizedTasks page.
>>>
>>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Jason, are you going to take this through the net tree?
> Can you fix up the long lines/space issues in the commit
> message if you do, please?
>
> thanks
> -- PMM

Fixed and apply in -net.

Thanks

Re: [Qemu-devel] [PATCH v2] Change net/socket.c to use socket_*() functions

[Ashijeet Acharya]

On Tue, Jun 21, 2016 at 7:19 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2016年06月20日 23:09, Peter Maydell wrote:
>>
>> On 20 June 2016 at 15:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>> On 18/06/2016 09:54, Ashijeet Acharya wrote:
>>>>
>>>> Use socket_*() functions from include/qemu/sockets.h instead of
>>>> listen()/bind()/ connect()/parse_host_port(). socket_*() fucntions are
>>>> QAPI based and this patch performs this api conversion since everything
>>>> will be using QAPI based sockets in the future. Also add a helper
>>>> function socket_address_to_string() in util/qemu-sockets.c which returns
>>>> the string representation of socket address. Thetask was listed on
>>>> http://wiki.qemu.org/BiteSizedTasks page.
>>>>
>>>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>>>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>>
>>> Jason, are you going to take this through the net tree?
>>
>> Can you fix up the long lines/space issues in the commit
>> message if you do, please?
>>
>> thanks
>> -- PMM
>
>
> Fixed and apply in -net.
>
> Thanks
Thanks a lot!

Ashijeet

Re: [Qemu-devel] [PATCH v2] Change net/socket.c to use socket_*() functions

[Daniel P. Berrange]

On Sat, Jun 18, 2016 at 01:24:02PM +0530, Ashijeet Acharya wrote:
> Use socket_*() functions from include/qemu/sockets.h instead of listen()/bind()/  connect()/parse_host_port(). socket_*() fucntions are QAPI based and this patch   performs this api conversion since everything will be using QAPI based sockets in the future. Also add a helper function socket_address_to_string() in              util/qemu-sockets.c which returns the string representation of socket address. Thetask was listed on http://wiki.qemu.org/BiteSizedTasks page.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  include/qemu/sockets.h | 16 ++++++++++++++-
>  net/socket.c           | 55 +++++++++++++++++++++++++-------------------------
>  util/qemu-sockets.c    | 36 +++++++++++++++++++++++++++++++++
>  3 files changed, 78 insertions(+), 29 deletions(-)
> 


> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 0d6cd1f..771b7db 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1151,3 +1151,39 @@ void qapi_copy_SocketAddress(SocketAddress **p_dest,
>      qmp_input_visitor_cleanup(qiv);
>      qobject_decref(obj);
>  }
> +
> +char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
> +{
> +    char *buf;
> +    InetSocketAddress *inet;
> +    char host_port[INET6_ADDRSTRLEN + 5 + 4];
> +
> +    switch (addr->type) {
> +    case SOCKET_ADDRESS_KIND_INET:
> +        inet = addr->u.inet.data;
> +        if (strchr(inet->host, ':') == NULL) {
> +            snprintf(host_port, sizeof(host_port), "%s:%s", inet->host,
> +                    inet->port);
> +            buf = g_strdup(host_port);
> +        } else {
> +            snprintf(host_port, sizeof(host_port), "[%s]:%s", inet->host,
> +                    inet->port);
> +            buf = g_strdup(host_port);

I see the patch is already queued, but really this should be changed to
not use a preallocated buffer. It should instead use g_strdup_printf()

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|