Libguestfs - Jul 2023 - [PATCH nbdkit v2 6/9] retry-request: Allow get_size operation to be retried

This plugin operation might need to do some real work (instead of just
fetching a number from memory), and so it might have to be retried.

In particular, changes to the curl plugin make .get_size into a
heavyweight operation, where previously it was done as a side-effect
of .open.  And so we must allow .get_size to be retried independent of
.open.
---
 filters/retry-request/retry-request.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/filters/retry-request/retry-request.c
b/filters/retry-request/retry-request.c
index e5b8344cd..8e3dd8246 100644
--- a/filters/retry-request/retry-request.c
+++ b/filters/retry-request/retry-request.c
@@ -141,6 +141,18 @@ retry_request_open (nbdkit_next_open *next, nbdkit_context
*nxdata,
   return r == 0 ? NBDKIT_HANDLE_NOT_NEEDED : NULL;
 }
 
+static int64_t
+retry_request_get_size (nbdkit_next *next, void *handle)
+{
+  int64_t r;
+  int *err = &errno;          /* used by the RETRY_* macros */
+
+  RETRY_START("get_size")
+    r = next->get_size (next);
+  RETRY_END;
+  return r;
+}
+
 static int
 retry_request_pread (nbdkit_next *next,
                      void *handle, void *buf, uint32_t count, uint64_t offset,
@@ -267,6 +279,7 @@ static struct nbdkit_filter filter = {
   .config            = retry_request_config,
   .config_help       = retry_request_config_help,
   .open              = retry_request_open,
+  .get_size          = retry_request_get_size,
   .pread             = retry_request_pread,
   .pwrite            = retry_request_pwrite,
   .trim              = retry_request_trim,
-- 
2.41.0

On Fri, Jul 28, 2023 at 06:17:50PM +0100, Richard W.M. Jones
wrote:
> This plugin operation might need to do some real work (instead of just
> fetching a number from memory), and so it might have to be retried.
> 
> In particular, changes to the curl plugin make .get_size into a
> heavyweight operation, where previously it was done as a side-effect
> of .open.  And so we must allow .get_size to be retried independent of
> .open.
> ---
>  filters/retry-request/retry-request.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
It feels a bit odd that .open isn't calling .get_size to cache it, but
it's hard to state whether that's a bug in the filter (for not
pre-caching .get_size), in the plugin (for not having .get_size ready
to go by the end of .open), in whatever other filter is stacked on top
of retry-request and not caching .get_size during its .open, or not a
bug at all.  Thus, while I'm slightly worried that this patch may be
papering over something instead of addressing root cause, I can't
pinpoint anything in specific where we might be going against our
documentation, and I can live with this going in.
> 
> diff --git a/filters/retry-request/retry-request.c
b/filters/retry-request/retry-request.c
> index e5b8344cd..8e3dd8246 100644
> --- a/filters/retry-request/retry-request.c
> +++ b/filters/retry-request/retry-request.c
> @@ -141,6 +141,18 @@ retry_request_open (nbdkit_next_open *next,
nbdkit_context *nxdata,
>    return r == 0 ? NBDKIT_HANDLE_NOT_NEEDED : NULL;
>  }
>  
> +static int64_t
> +retry_request_get_size (nbdkit_next *next, void *handle)
> +{
> +  int64_t r;
> +  int *err = &errno;          /* used by the RETRY_* macros */
This is the reason why I'm reluctant to say whether this is the right
approach - if .get_size() is encountered during a .pread, there is no
guarantee that we pass a correct errno value back if the pread fails
because .get_size failed.  Ensuring that .get_size is cached during
.open guarantees that we have a valid size for all subsequent
operations, without needing to worry about what happens to errno.
> +
> +  RETRY_START("get_size")
> +    r = next->get_size (next);
> +  RETRY_END;
> +  return r;
> +}
> +
>  static int
>  retry_request_pread (nbdkit_next *next,
>                       void *handle, void *buf, uint32_t count, uint64_t
offset,
> @@ -267,6 +279,7 @@ static struct nbdkit_filter filter = {
>    .config            = retry_request_config,
>    .config_help       = retry_request_config_help,
>    .open              = retry_request_open,
> +  .get_size          = retry_request_get_size,
>    .pread             = retry_request_pread,
>    .pwrite            = retry_request_pwrite,
>    .trim              = retry_request_trim,
> -- 
> 2.41.0
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org