[Libguestfs] [nbdkit PATCH v3 03/16] server: Move default_exportname from handle to connection

[Richard W.M. Jones]

On Fri, Mar 05, 2021 at 05:31:07PM -0600, Eric Blake wrote:
> struct handle represents things that can differ per export (such as
> the results of .can_write), but .default_exportname is really
> something that should be global to all possible exports of the
> connection.  Moving it also gets rid of the leaky abstraction where
> NBD_OPT_STARTTLS has to reset the cached default when changing TLS
> even though there is no current handle at that time.  It also lets us
> simplify reset_handle(), as it no longer tracks allocated memory, and
> prepares for an upcoming patch refactoring how backend_open operates.
> ---
>  server/internal.h                    |  6 ++----
>  server/backend.c                     |  8 ++++----
>  server/connections.c                 | 15 ++++++++++++---
>  server/protocol-handshake-newstyle.c |  7 +++----
>  4 files changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/server/internal.h b/server/internal.h
> index 906f0690..1f37703f 100644
> --- a/server/internal.h
> +++ b/server/internal.h
> @@ -211,8 +211,6 @@ struct handle {
> 
>    unsigned char state;  /* Bitmask of HANDLE_* values */
> 
> -  char *default_exportname;
> -
>    uint64_t exportsize;
>    int can_write;
>    int can_flush;
> @@ -231,8 +229,6 @@ reset_handle (struct handle *h)
>  {
>    h->handle = NULL;
>    h->state = 0;
> -  free (h->default_exportname);
> -  h->default_exportname = NULL;
>    h->exportsize = -1;
>    h->can_write = -1;
>    h->can_flush = -1;
> @@ -261,6 +257,8 @@ struct connection {
>    struct handle *handles;       /* One per plugin and filter. */
>    size_t nr_handles;
> 
> +  char **default_exportname;    /* One per plugin and filter. */
> +
>    uint32_t cflags;
>    uint16_t eflags;
>    bool handshake_complete;
> diff --git a/server/backend.c b/server/backend.c
> index e9fcd696..d08a2b6b 100644
> --- a/server/backend.c
> +++ b/server/backend.c
> @@ -193,7 +193,7 @@ backend_default_export (struct backend *b, int readonly)
>    controlpath_debug ("%s: default_export readonly=%d tls=%d",
>                       b->name, readonly, conn->using_tls);
> 
> -  if (h->default_exportname == NULL) {
> +  if (conn->default_exportname[b->i] == NULL) {
>      assert (h->handle == NULL);
>      assert ((h->state & HANDLE_OPEN) == 0);
>      s = b->default_export (b, readonly, conn->using_tls);
> @@ -205,12 +205,12 @@ backend_default_export (struct backend *b, int readonly)
>      }
>      if (s) {
>        /* Best effort caching */
> -      h->default_exportname = strdup (s);
> -      if (h->default_exportname == NULL)
> +      conn->default_exportname[b->i] = strdup (s);
> +      if (conn->default_exportname[b->i] == NULL)
>          return s;
>      }
>    }
> -  return h->default_exportname;
> +  return conn->default_exportname[b->i];
>  }
> 
>  int
> diff --git a/server/connections.c b/server/connections.c
> index 38b32742..b56f89f8 100644
> --- a/server/connections.c
> +++ b/server/connections.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2013-2020 Red Hat Inc.
> + * Copyright (C) 2013-2021 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -263,6 +263,14 @@ new_connection (int sockin, int sockout, int nworkers)
>    for_each_backend (b)
>      reset_handle (get_handle (conn, b->i));
> 
> +  conn->default_exportname = calloc (top->i + 1,
> +                                     sizeof *conn->default_exportname);
> +  if (conn->default_exportname == NULL) {
> +    perror ("malloc");
> +    free (conn->handles);
> +    goto error1;
> +  }
> +
>    conn->status = 1;
>    conn->nworkers = nworkers;
>    if (nworkers) {
> @@ -330,6 +338,7 @@ new_connection (int sockin, int sockout, int nworkers)
>    if (conn->status_pipe[1] >= 0)
>      close (conn->status_pipe[1]);
>    free (conn->handles);
> +  free (conn->default_exportname);
> 
>   error1:
>    pthread_mutex_destroy (&conn->request_lock);
> @@ -373,9 +382,9 @@ free_connection (struct connection *conn)
>    free (conn->exportname_from_set_meta_context);
>    free_interns ();
> 
> -  /* This is needed in order to free a field in struct handle. */
>    for_each_backend (b)
> -    reset_handle (get_handle (conn, b->i));
> +    free (conn->default_exportname[b->i]);
> +  free (conn->default_exportname);
>    free (conn->handles);
> 
>    free (conn);
> diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c
> index 0a76a814..fb4a93bc 100644
> --- a/server/protocol-handshake-newstyle.c
> +++ b/server/protocol-handshake-newstyle.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2013-2020 Red Hat Inc.
> + * Copyright (C) 2013-2021 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -497,9 +497,8 @@ negotiate_handshake_newstyle_options (void)
>          debug ("using TLS on this connection");
>          /* Wipe out any cached default export name. */
>          for_each_backend (b) {
> -          struct handle *h = get_handle (conn, b->i);
> -          free (h->default_exportname);
> -          h->default_exportname = NULL;
> +          free (conn->default_exportname[b->i]);
> +          conn->default_exportname[b->i] = NULL;
>          }
>        }
>        break;

ACK

I think this patch is independent from the rest, so
we should probably push this early.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org