[Libguestfs] [nbdkit PATCH 1/2] retry: Add in retry support during .open

[Richard W.M. Jones]

On Fri, Jan 27, 2023 at 02:41:22PM -0600, Eric Blake wrote:
> Now that a filter can open a backend as many times as it wants,
> there's no longer a technical reason we can't retry .open.  However,
> adding retry logic here does mean we have to weaken an assert in the
> server backend code, since prepare can now be reached more than once.
> 
> Test coverage will be added in a separate patch, so that it becomes
> easy to swap patch order and see that the test fails without this
> patch.
> 
> See also https://bugzilla.redhat.com/show_bug.cgi?id=1841820
> ---
>  server/backend.c      |  7 +++-
>  filters/retry/retry.c | 98 +++++++++++++++++++++++++------------------
>  2 files changed, 63 insertions(+), 42 deletions(-)
> 
> diff --git a/server/backend.c b/server/backend.c
> index 3bbba601..45889ea9 100644
> --- a/server/backend.c
> +++ b/server/backend.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2013-2021 Red Hat Inc.
> + * Copyright (C) 2013-2023 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -312,7 +312,10 @@ backend_prepare (struct context *c)
>    struct backend *b = c->b;
> 
>    assert (c->handle);
> -  assert ((c->state & (HANDLE_OPEN | HANDLE_CONNECTED)) == HANDLE_OPEN);
> +  assert (c->state & HANDLE_OPEN);
> +
> +  if (c->state & HANDLE_CONNECTED)
> +    return 0;

OK because CONNECTED means the handle has been opened & prepared already.

>    /* Call these in order starting from the filter closest to the
>     * plugin, similar to typical .open order.  But remember that
> diff --git a/filters/retry/retry.c b/filters/retry/retry.c
> index fb1d0526..7abf74c4 100644
> --- a/filters/retry/retry.c
> +++ b/filters/retry/retry.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2019-2021 Red Hat Inc.
> + * Copyright (C) 2019-2023 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -113,45 +113,6 @@ struct retry_handle {
>    bool open;
>  };
> 
> -static void *
> -retry_open (nbdkit_next_open *next, nbdkit_context *nxdata,
> -            int readonly, const char *exportname, int is_tls)
> -{
> -  struct retry_handle *h;
> -
> -  if (next (nxdata, readonly, exportname) == -1)
> -    return NULL;
> -
> -  h = malloc (sizeof *h);
> -  if (h == NULL) {
> -    nbdkit_error ("malloc: %m");
> -    return NULL;
> -  }
> -
> -  h->readonly = readonly;
> -  h->exportname = strdup (exportname);
> -  h->context = nxdata;
> -  if (h->exportname == NULL) {
> -    nbdkit_error ("strdup: %m");
> -    free (h);
> -    return NULL;
> -  }
> -  h->reopens = 0;
> -  h->open = true;
> -
> -  return h;
> -}
> -
> -static void
> -retry_close (void *handle)
> -{
> -  struct retry_handle *h = handle;
> -
> -  nbdkit_debug ("reopens needed: %u", h->reopens);
> -  free (h->exportname);
> -  free (h);
> -}
> -
>  /* This function encapsulates the common retry logic used across all
>   * data commands.  If it returns true then the data command will retry
>   * the operation.  ???struct retry_data??? is stack data saved between
> @@ -247,6 +208,63 @@ do_retry (struct retry_handle *h, struct retry_data *data,
>    return true;
>  }
> 
> +static void *
> +retry_open (nbdkit_next_open *next, nbdkit_context *nxdata,
> +            int readonly, const char *exportname, int is_tls)
> +{
> +  struct retry_handle *h;
> +  struct retry_data data = {0};
> +
> +  h = malloc (sizeof *h);
> +  if (h == NULL) {
> +    nbdkit_error ("malloc: %m");
> +    return NULL;
> +  }
> +
> +  h->readonly = readonly;
> +  h->exportname = strdup (exportname);
> +  h->context = nxdata;
> +  if (h->exportname == NULL) {
> +    nbdkit_error ("strdup: %m");
> +    free (h);
> +    return NULL;
> +  }
> +  h->reopens = 0;
> +
> +  if (next (nxdata, readonly, exportname) != -1)
> +    h->open = true;
> +  else {
> +    /* Careful - our .open must not return a handle unless do_retry()
> +     * works, as the caller's next action will be calling .get_size
> +     * and similar probe functions which we do not bother to wire up
> +     * into retry logic because they only need to be used right after
> +     * connecting.
> +     */
> +    nbdkit_next *next_handle = NULL;
> +    int err = ESHUTDOWN;
> +
> +    while (! h->open && do_retry (h, &data, &next_handle, "open", &err))
> +      ;
> +
> +    if (! h->open) {
> +      free (h->exportname);
> +      free (h);
> +      return NULL;
> +    }
> +  }
> +  return h;
> +}
> +
> +static void
> +retry_close (void *handle)
> +{
> +  struct retry_handle *h = handle;
> +
> +  nbdkit_debug ("reopens needed: %u", h->reopens);
> +  free (h->exportname);
> +  free (h);
> +}
> +
>  static int
>  retry_pread (nbdkit_next *next,
>               void *handle, void *buf, uint32_t count, uint64_t offset,
> -- 

Seems OK, ACK (modulo your comment about leaking memory
in your reply).

I checked the documentation of the filter and it doesn't mention the
limitation, so it seems we don't need to adjust the documentation at
all.

I will try this out with the ssh filter once the bug moves into post.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html