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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
