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

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;

   /* 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,
-- 
2.39.1

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
> ---
> +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);
Uninit memory...
> +  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 {
and h->open has not been assigned if we get here...
> +    /* 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))
...even though I assumed it was zero-initialized here.  Will fix.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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