[Qemu-devel] [PATCH 0/2] qemu-sockets: remove useless functions

[Qemu-devel] [PATCH 0/2] qemu-sockets: remove useless functions

[Cao jin]

The commit message maybe not so accurate, welcome to the comments.

Cc: Daniel P. Berrange <berrange@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>

Cao jin (2):
  util/qemu-sockets: shoot inet_nonblocking_connect()
  util/qemu-sockets: shoot unix_nonblocking_connect()

 include/qemu/sockets.h |  6 ------
 util/qemu-sockets.c    | 46 ----------------------------------------------
 2 files changed, 52 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] util/qemu-sockets: shoot inet_nonblocking_connect()

[Cao jin]

It is never used, and now all connect is nonblocking via
inet_connect_addr().

Cc: Daniel P. Berrange <berrange@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 include/qemu/sockets.h |  3 ---
 util/qemu-sockets.c    | 30 ------------------------------
 2 files changed, 33 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 5fe01fb..2cbe643 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -36,9 +36,6 @@ InetSocketAddress *inet_parse(const char *str, Error **errp);
 int inet_listen(const char *str, char *ostr, int olen,
                 int socktype, int port_offset, Error **errp);
 int inet_connect(const char *str, Error **errp);
-int inet_nonblocking_connect(const char *str,
-                             NonBlockingConnectHandler *callback,
-                             void *opaque, Error **errp);
 
 NetworkAddressFamily inet_netfamily(int family);
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index fb83d48..88b822a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -674,36 +674,6 @@ int inet_connect(const char *str, Error **errp)
     return sock;
 }
 
-/**
- * Create a non-blocking socket and connect it to an address.
- * Calls the callback function with fd in case of success or -1 in case of
- * error.
- *
- * @str: address string
- * @callback: callback function that is called when connect completes,
- *            cannot be NULL.
- * @opaque: opaque for callback function
- * @errp: set in case of an error
- *
- * Returns: -1 on immediate error, file descriptor on success.
- **/
-int inet_nonblocking_connect(const char *str,
-                             NonBlockingConnectHandler *callback,
-                             void *opaque, Error **errp)
-{
-    int sock = -1;
-    InetSocketAddress *addr;
-
-    g_assert(callback != NULL);
-
-    addr = inet_parse(str, errp);
-    if (addr != NULL) {
-        sock = inet_connect_saddr(addr, errp, callback, opaque);
-        qapi_free_InetSocketAddress(addr);
-    }
-    return sock;
-}
-
 #ifndef _WIN32
 
 static int unix_listen_saddr(UnixSocketAddress *saddr,
-- 
2.1.0

[Qemu-devel] [PATCH 2/2] util/qemu-sockets: shoot unix_nonblocking_connect()

[Cao jin]

It is never used, and now all connect is nonblocking via
inet_connect_addr().

Cc: Daniel P. Berrange <berrange@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 include/qemu/sockets.h |  3 ---
 util/qemu-sockets.c    | 16 ----------------
 2 files changed, 19 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 2cbe643..28a28c0 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -41,9 +41,6 @@ NetworkAddressFamily inet_netfamily(int family);
 
 int unix_listen(const char *path, char *ostr, int olen, Error **errp);
 int unix_connect(const char *path, Error **errp);
-int unix_nonblocking_connect(const char *str,
-                             NonBlockingConnectHandler *callback,
-                             void *opaque, Error **errp);
 
 SocketAddress *socket_parse(const char *str, Error **errp);
 int socket_connect(SocketAddress *addr, Error **errp,
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 88b822a..0962646 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -863,22 +863,6 @@ int unix_connect(const char *path, Error **errp)
 }
 
 
-int unix_nonblocking_connect(const char *path,
-                             NonBlockingConnectHandler *callback,
-                             void *opaque, Error **errp)
-{
-    UnixSocketAddress *saddr;
-    int sock = -1;
-
-    g_assert(callback != NULL);
-
-    saddr = g_new0(UnixSocketAddress, 1);
-    saddr->path = g_strdup(path);
-    sock = unix_connect_saddr(saddr, errp, callback, opaque);
-    qapi_free_UnixSocketAddress(saddr);
-    return sock;
-}
-
 SocketAddress *socket_parse(const char *str, Error **errp)
 {
     SocketAddress *addr;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/2] qemu-sockets: remove useless functions

[Cao jin]

Got delivery failure feedback. Ping to ensure maintainer see this one

On 07/21/2016 06:33 PM, Cao jin wrote:
> The commit message maybe not so accurate, welcome to the comments.
>
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
>
> Cao jin (2):
>    util/qemu-sockets: shoot inet_nonblocking_connect()
>    util/qemu-sockets: shoot unix_nonblocking_connect()
>
>   include/qemu/sockets.h |  6 ------
>   util/qemu-sockets.c    | 46 ----------------------------------------------
>   2 files changed, 52 deletions(-)
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH 0/2] qemu-sockets: remove useless functions

[Paolo Bonzini]

On 21/07/2016 12:33, Cao jin wrote:
> The commit message maybe not so accurate, welcome to the comments.
> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> 
> Cao jin (2):
>   util/qemu-sockets: shoot inet_nonblocking_connect()
>   util/qemu-sockets: shoot unix_nonblocking_connect()

It's okay, I'll rephrase it to

It is never used; all nonblocking connect now goes through
inet_connect_addr().

and

It is never used; all nonblocking connect now goes through
unix_connect_addr().

respectively.

Thanks,

Paolo

>  include/qemu/sockets.h |  6 ------
>  util/qemu-sockets.c    | 46 ----------------------------------------------
>  2 files changed, 52 deletions(-)
> 

Re: [Qemu-devel] [PATCH 1/2] util/qemu-sockets: shoot inet_nonblocking_connect()

[Eric Blake]

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

On 07/21/2016 04:33 AM, Cao jin wrote:
> It is never used, and now all connect is nonblocking via
> inet_connect_addr().

Maybe the subject line could be:

util: Drop unused inet_unblocking_connect()

but what you have isn't too bad.

> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  include/qemu/sockets.h |  3 ---
>  util/qemu-sockets.c    | 30 ------------------------------
>  2 files changed, 33 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] util/qemu-sockets: shoot unix_nonblocking_connect()

[Eric Blake]

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

On 07/21/2016 04:33 AM, Cao jin wrote:
> It is never used, and now all connect is nonblocking via
> inet_connect_addr().
> 

Could be squashed with 1/2.  In fact, if you squash it, I'd title the patch:

util: Drop unused *_nonblocking_connect() functions

You may also want to call out which commit id rendered the functions unused.

> Cc: Daniel P. Berrange <berrange@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  include/qemu/sockets.h |  3 ---
>  util/qemu-sockets.c    | 16 ----------------
>  2 files changed, 19 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] util/qemu-sockets: shoot unix_nonblocking_connect()

[Daniel P. Berrange]

On Thu, Jul 21, 2016 at 08:42:25AM -0600, Eric Blake wrote:
> On 07/21/2016 04:33 AM, Cao jin wrote:
> > It is never used, and now all connect is nonblocking via
> > inet_connect_addr().
> > 
> 
> Could be squashed with 1/2.  In fact, if you squash it, I'd title the patch:
> 
> util: Drop unused *_nonblocking_connect() functions
> 
> You may also want to call out which commit id rendered the functions unused.

Well once those two functions are dropped the only other place accepting
NonBlockingConnectHandler is the socket_connect() method. Since nearly
everything is converted to QIOChannel now, there's only one caller of
socket_connect() left, and that's net/socket.c

Any newly written code which needs a non-blocking connect should use the
QIOChannel code, so I don't see any further usage of socket_connect()
being added.

IOW, we can rip out NonBlockingConnectHandler as a concept entirely, not
merely drop the *_nonblocking_connect() methods.

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 :|

Re: [Qemu-devel] [PATCH 2/2] util/qemu-sockets: shoot unix_nonblocking_connect()

[Daniel P. Berrange]

On Fri, Jul 22, 2016 at 06:34:11PM +0800, Cao jin wrote:
> Hi Daniel
> 
> On 07/21/2016 11:39 PM, Daniel P. Berrange wrote:
> > On Thu, Jul 21, 2016 at 08:42:25AM -0600, Eric Blake wrote:
> > > On 07/21/2016 04:33 AM, Cao jin wrote:
> > > > It is never used, and now all connect is nonblocking via
> > > > inet_connect_addr().
> > > > 
> > > 
> > > Could be squashed with 1/2.  In fact, if you squash it, I'd title the patch:
> > > 
> > > util: Drop unused *_nonblocking_connect() functions
> > > 
> > > You may also want to call out which commit id rendered the functions unused.
> > 
> > Well once those two functions are dropped the only other place accepting
> > NonBlockingConnectHandler is the socket_connect() method. Since nearly
> > everything is converted to QIOChannel now, there's only one caller of
> > socket_connect() left, and that's net/socket.c
> > 
> > Any newly written code which needs a non-blocking connect should use the
> > QIOChannel code, so I don't see any further usage of socket_connect()
> > being added.
> > 
> > IOW, we can rip out NonBlockingConnectHandler as a concept entirely, not
> > merely drop the *_nonblocking_connect() methods.
> > 
> 
> I don't quite follow the "rip out NonBlockingConnectHandler" thing.
> According what I learned from code, we offered non-blocking connection
> mechanism, but it seems nobody use it(all callers of socket_connect() set
> callback as NULL), so, do you mean removing this mechanism?

Yes, remove it all, as it is no longer needed.

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 :|

Re: [Qemu-devel] [PATCH 2/2] util/qemu-sockets: shoot unix_nonblocking_connect()

[Cao jin]

Hi Daniel

On 07/21/2016 11:39 PM, Daniel P. Berrange wrote:
> On Thu, Jul 21, 2016 at 08:42:25AM -0600, Eric Blake wrote:
>> On 07/21/2016 04:33 AM, Cao jin wrote:
>>> It is never used, and now all connect is nonblocking via
>>> inet_connect_addr().
>>>
>>
>> Could be squashed with 1/2.  In fact, if you squash it, I'd title the patch:
>>
>> util: Drop unused *_nonblocking_connect() functions
>>
>> You may also want to call out which commit id rendered the functions unused.
>
> Well once those two functions are dropped the only other place accepting
> NonBlockingConnectHandler is the socket_connect() method. Since nearly
> everything is converted to QIOChannel now, there's only one caller of
> socket_connect() left, and that's net/socket.c
>
> Any newly written code which needs a non-blocking connect should use the
> QIOChannel code, so I don't see any further usage of socket_connect()
> being added.
>
> IOW, we can rip out NonBlockingConnectHandler as a concept entirely, not
> merely drop the *_nonblocking_connect() methods.
>

I don't quite follow the "rip out NonBlockingConnectHandler" thing. 
According what I learned from code, we offered non-blocking connection 
mechanism, but it seems nobody use it(all callers of socket_connect() 
set callback as NULL), so, do you mean removing this mechanism?

more explanation will be much appreciated:)

> Regards,
> Daniel
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH 2/2] util/qemu-sockets: shoot unix_nonblocking_connect()

[Daniel P. Berrange]

On Fri, Jul 22, 2016 at 06:43:51PM +0800, Cao jin wrote:
> 
> 
> On 07/22/2016 06:30 PM, Daniel P. Berrange wrote:
> > On Fri, Jul 22, 2016 at 06:34:11PM +0800, Cao jin wrote:
> > > Hi Daniel
> > > 
> > > On 07/21/2016 11:39 PM, Daniel P. Berrange wrote:
> > > > On Thu, Jul 21, 2016 at 08:42:25AM -0600, Eric Blake wrote:
> > > > > On 07/21/2016 04:33 AM, Cao jin wrote:
> > > > > > It is never used, and now all connect is nonblocking via
> > > > > > inet_connect_addr().
> > > > > > 
> > > > > 
> > > > > Could be squashed with 1/2.  In fact, if you squash it, I'd title the patch:
> > > > > 
> > > > > util: Drop unused *_nonblocking_connect() functions
> > > > > 
> > > > > You may also want to call out which commit id rendered the functions unused.
> > > > 
> > > > Well once those two functions are dropped the only other place accepting
> > > > NonBlockingConnectHandler is the socket_connect() method. Since nearly
> > > > everything is converted to QIOChannel now, there's only one caller of
> > > > socket_connect() left, and that's net/socket.c
> > > > 
> > > > Any newly written code which needs a non-blocking connect should use the
> > > > QIOChannel code, so I don't see any further usage of socket_connect()
> > > > being added.
> > > > 
> > > > IOW, we can rip out NonBlockingConnectHandler as a concept entirely, not
> > > > merely drop the *_nonblocking_connect() methods.
> > > > 
> > > 
> > > I don't quite follow the "rip out NonBlockingConnectHandler" thing.
> > > According what I learned from code, we offered non-blocking connection
> > > mechanism, but it seems nobody use it(all callers of socket_connect() set
> > > callback as NULL), so, do you mean removing this mechanism?
> > 
> > Yes, remove it all, as it is no longer needed.
> > 
> 
> Thanks for clarifying. Actually, I am still curious why nobody want to use
> this mechanism, is there any disadvantage? And why this mechanism is
> introduced in

As mentioned previously it is obsolete as all new code will use the QIOChannel
APIs which already provide non-blocking connect in a different manner. The
qemu-sockets non-blocking code never worked properly in the first place
because it calls getaddrinfo() which blocks on DNS lookups.

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 :|

Re: [Qemu-devel] [PATCH 2/2] util/qemu-sockets: shoot unix_nonblocking_connect()

[Cao jin]

On 07/22/2016 06:30 PM, Daniel P. Berrange wrote:
> On Fri, Jul 22, 2016 at 06:34:11PM +0800, Cao jin wrote:
>> Hi Daniel
>>
>> On 07/21/2016 11:39 PM, Daniel P. Berrange wrote:
>>> On Thu, Jul 21, 2016 at 08:42:25AM -0600, Eric Blake wrote:
>>>> On 07/21/2016 04:33 AM, Cao jin wrote:
>>>>> It is never used, and now all connect is nonblocking via
>>>>> inet_connect_addr().
>>>>>
>>>>
>>>> Could be squashed with 1/2.  In fact, if you squash it, I'd title the patch:
>>>>
>>>> util: Drop unused *_nonblocking_connect() functions
>>>>
>>>> You may also want to call out which commit id rendered the functions unused.
>>>
>>> Well once those two functions are dropped the only other place accepting
>>> NonBlockingConnectHandler is the socket_connect() method. Since nearly
>>> everything is converted to QIOChannel now, there's only one caller of
>>> socket_connect() left, and that's net/socket.c
>>>
>>> Any newly written code which needs a non-blocking connect should use the
>>> QIOChannel code, so I don't see any further usage of socket_connect()
>>> being added.
>>>
>>> IOW, we can rip out NonBlockingConnectHandler as a concept entirely, not
>>> merely drop the *_nonblocking_connect() methods.
>>>
>>
>> I don't quite follow the "rip out NonBlockingConnectHandler" thing.
>> According what I learned from code, we offered non-blocking connection
>> mechanism, but it seems nobody use it(all callers of socket_connect() set
>> callback as NULL), so, do you mean removing this mechanism?
>
> Yes, remove it all, as it is no longer needed.
>

Thanks for clarifying. Actually, I am still curious why nobody want to 
use this mechanism, is there any disadvantage? And why this mechanism is 
introduced in

> Regards,
> Daniel
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH 2/2] util/qemu-sockets: shoot unix_nonblocking_connect()

[Cao jin]

On 07/22/2016 06:38 PM, Daniel P. Berrange wrote:
> On Fri, Jul 22, 2016 at 06:43:51PM +0800, Cao jin wrote:
>>
>>
>> On 07/22/2016 06:30 PM, Daniel P. Berrange wrote:
>>> On Fri, Jul 22, 2016 at 06:34:11PM +0800, Cao jin wrote:
>>>> Hi Daniel
>>>>
>>>> On 07/21/2016 11:39 PM, Daniel P. Berrange wrote:
>>>>> On Thu, Jul 21, 2016 at 08:42:25AM -0600, Eric Blake wrote:
>>>>>> On 07/21/2016 04:33 AM, Cao jin wrote:
>>>>>>> It is never used, and now all connect is nonblocking via
>>>>>>> inet_connect_addr().
>>>>>>>
>>>>>>
>>>>>> Could be squashed with 1/2.  In fact, if you squash it, I'd title the patch:
>>>>>>
>>>>>> util: Drop unused *_nonblocking_connect() functions
>>>>>>
>>>>>> You may also want to call out which commit id rendered the functions unused.
>>>>>
>>>>> Well once those two functions are dropped the only other place accepting
>>>>> NonBlockingConnectHandler is the socket_connect() method. Since nearly
>>>>> everything is converted to QIOChannel now, there's only one caller of
>>>>> socket_connect() left, and that's net/socket.c
>>>>>
>>>>> Any newly written code which needs a non-blocking connect should use the
>>>>> QIOChannel code, so I don't see any further usage of socket_connect()
>>>>> being added.
>>>>>
>>>>> IOW, we can rip out NonBlockingConnectHandler as a concept entirely, not
>>>>> merely drop the *_nonblocking_connect() methods.
>>>>>
>>>>
>>>> I don't quite follow the "rip out NonBlockingConnectHandler" thing.
>>>> According what I learned from code, we offered non-blocking connection
>>>> mechanism, but it seems nobody use it(all callers of socket_connect() set
>>>> callback as NULL), so, do you mean removing this mechanism?
>>>
>>> Yes, remove it all, as it is no longer needed.
>>>
>>
>> Thanks for clarifying. Actually, I am still curious why nobody want to use
>> this mechanism, is there any disadvantage? And why this mechanism is
>> introduced in
>
> As mentioned previously it is obsolete as all new code will use the QIOChannel
> APIs which already provide non-blocking connect in a different manner. The
> qemu-sockets non-blocking code never worked properly in the first place
> because it calls getaddrinfo() which blocks on DNS lookups.
>

Aha! I see! And also I see the comment you left in the code:
     /* socket_connect() does a non-blocking connect(), but it
      * still blocks in DNS lookups, so we must use a thread */

Thanks very much, and I think maybe I can do this cleanup work:)

> Regards,
> Daniel
>

-- 
Yours Sincerely,

Cao jin