| Message ID | 1469097213-26441-3-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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
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 >
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
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 >
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 >
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;
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(-)