Re: [Qemu-devel] [PATCH 1/2] nbd: Interface tweak of nbd_client_new

["Daniel P. Berrange" <berrange@redhat.com>]

On Wed, Dec 30, 2015 at 01:49:25PM +0800, Fam Zheng wrote:
> In preparation for an async implementation, introduce a callback and
> move the shutdown/close to the function.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev-nbd.c      |  5 ++---
>  include/block/nbd.h |  6 ++++--
>  nbd.c               | 20 +++++++++++++++-----
>  qemu-nbd.c          | 16 +++++++++-------
>  4 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index bcdd18b..f708e0f 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -27,9 +27,8 @@ static void nbd_accept(void *opaque)
>      socklen_t addr_len = sizeof(addr);
>  
>      int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
> -    if (fd >= 0 && !nbd_client_new(NULL, fd, nbd_client_put)) {
> -        shutdown(fd, 2);
> -        close(fd);
> +    if (fd >= 0) {
> +        nbd_client_new(NULL, fd, nbd_client_put, NULL);
>      }
>  }
>  
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 65f409d..11194e0 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -98,8 +98,10 @@ NBDExport *nbd_export_find(const char *name);
>  void nbd_export_set_name(NBDExport *exp, const char *name);
>  void nbd_export_close_all(void);
>  
> -NBDClient *nbd_client_new(NBDExport *exp, int csock,
> -                          void (*close)(NBDClient *));
> +typedef void (*NBDClientNewCB)(NBDExport *exp, int csock, int ret);
> +void nbd_client_new(NBDExport *exp, int csock,
> +                    void (*close_fn)(NBDClient *),
> +                    NBDClientNewCB cb);
>  void nbd_client_get(NBDClient *client);
>  void nbd_client_put(NBDClient *client);
>  
> diff --git a/nbd.c b/nbd.c
> index b3d9654..bcb79d4 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -1475,9 +1475,13 @@ static void nbd_update_can_read(NBDClient *client)
>      }
>  }
>  
> -NBDClient *nbd_client_new(NBDExport *exp, int csock,
> -                          void (*close)(NBDClient *))
> +/* Create and initialize a new client. If it fails, @csock is closed.
> + * cb will be called with the status code when done. */
> +void nbd_client_new(NBDExport *exp, int csock,
> +                    void (*close_fn)(NBDClient *),
> +                    NBDClientNewCB cb)
>  {
> +    int ret = 0;
>      NBDClient *client;
>      client = g_malloc0(sizeof(NBDClient));
>      client->refcount = 1;
> @@ -1485,10 +1489,13 @@ NBDClient *nbd_client_new(NBDExport *exp, int csock,
>      client->sock = csock;
>      client->can_read = true;
>      if (nbd_send_negotiate(client)) {
> +        shutdown(csock, 2);
> +        close(csock);
>          g_free(client);
> -        return NULL;
> +        ret = -1;
> +        goto out;

If you simply make this failure code branch call close_fn() then I
think you can adding needing the new NBDClientNewCB entirely if....

> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 65dc30c..bdec228 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -319,6 +319,14 @@ static void nbd_client_closed(NBDClient *client)
>      nbd_client_put(client);
>  }
>  
> +static void nbd_client_new_cb(NBDExport *exp, int fd, int ret)
> +{
> +    if (ret == 0) {
> +        nb_fds++;
> +        nbd_update_server_fd_handler(server_fd);
> +    }
> +}
> +
>  static void nbd_accept(void *opaque)
>  {
>      struct sockaddr_in addr;
> @@ -335,13 +343,7 @@ static void nbd_accept(void *opaque)
>          return;
>      }
>  
> -    if (nbd_client_new(exp, fd, nbd_client_closed)) {
> -        nb_fds++;
> -        nbd_update_server_fd_handler(server_fd);
> -    } else {
> -        shutdown(fd, 2);
> -        close(fd);
> -    }
> +    nbd_client_new(exp, fd, nbd_client_closed, nbd_client_new_cb);

...you make this do

    nb_fds++;
    nbd_update_server_fd_handler(server_fd);
    nbd_client_new(exp, fd, nbd_client_closed, nbd_client_new_cb);

ie, you once guarantee that *every* invocation of nbd_client_new()
will eventually lead to a call to 'nbd_client_closed', you can
unconditionally increment nb_fds before calling nbd_client_new.


This has the added benefit in that the 'nb_fds' count now takes
account of client connections that are in the negotiate phase,
whereas your approach allows for an unlimited number of clients
to be in the negotiate phase, only limiting them post-negotiate

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