[Libguestfs] [nbdkit PATCH 1/2] blocksize: Refactor to per-handle constraints

[Richard W.M. Jones]

On Wed, Feb 23, 2022 at 09:45:32AM -0600, Eric Blake wrote:
> An upcoming patch wants to make the blocksize filter more responsive
> to the actual .block_size limits from the underlying plugin.  But
> those limits can differ per export (for example, qemu can export
> multiple images, where one backed by a file has minblock 1, but
> another backed by encryption has minblock 512).
> 
> To do that, we need to track block size constraints per handle, rather
> than globally.  At this point, no functional change is intended; the
> change is merely the refactoring to get per-handle values, and
> deferring the settling of defaults not specified during .config to now
> be initialized during .prepare rather than .config_complete.  The next
> patch can then modify .block_size to actually honor the underlying
> plugin constraints.
> 
> ---
> 
> I wrote this patch by temporarily renaming the globals min_block and
> so on, to let the compiler flag where we want to use the h->minblock
> per-handle variant.  If we want to leave the globals named slightly
> differently for safety, let me know, and I can reinstate that.

It may be an idea to do that.  In nbdkit-blocksize-policy-filter I
called the globals “config_maximum” etc.

> @@ -114,28 +122,20 @@ blocksize_config_complete (nbdkit_next_config_complete *next,
>        return -1;
>      }
>    }
> -  else
> -    minblock = 1;
> 
> -  if (maxdata) {
> +  if (maxdata && minblock) {
>      if (maxdata & (minblock - 1)) {
>        nbdkit_error ("maxdata must be a multiple of %u", minblock);
>        return -1;
>      }
>    }
> -  else if (maxlen)
> -    maxdata = MIN (maxlen, 64 * 1024 * 1024);
> -  else
> -    maxdata = 64 * 1024 * 1024;
> 
> -  if (maxlen) {
> +  if (maxlen && minblock) {
>      if (maxlen & (minblock - 1)) {
>        nbdkit_error ("maxlen must be a multiple of %u", minblock);
>        return -1;
>      }
>    }
> -  else
> -    maxlen = -minblock;

This seems like more than a simple renaming change ...

>    return next (nxdata);
>  }
> @@ -145,16 +145,66 @@ blocksize_config_complete (nbdkit_next_config_complete *next,
>    "maxdata=<SIZE>       Maximum size for read/write (default 64M).\n" \
>    "maxlen=<SIZE>        Maximum size for trim/zero (default 4G-minblock)."
> 
> +static void *
> +blocksize_open (nbdkit_next_open *next, nbdkit_context *nxdata,
> +                int readonly, const char *exportname, int is_tls)
> +{
> +  struct blocksize_handle *h;
> +
> +  if (next (nxdata, readonly, exportname) == -1)
> +    return NULL;
> +
> +  h = malloc (sizeof *h);
> +  if (h == NULL) {
> +    nbdkit_error ("malloc: %m");
> +    return NULL;
> +  }
> +
> +  h->minblock = minblock;
> +  h->maxdata = maxdata;
> +  h->maxlen = maxlen;
> +  return h;
> +}
> +
> +static int
> +blocksize_prepare (nbdkit_next *next, void *handle,
> +                   int readonly)
> +{
> +  struct blocksize_handle *h = handle;
> +
> +  if (h->minblock == 0)
> +    h->minblock = 1;
> +  if (h->maxdata == 0) {
> +    if (h->maxlen)
> +      h->maxdata = MIN (h->maxlen, 64 * 1024 * 1024);
> +    else
> +      h->maxdata = 64 * 1024 * 1024;
> +  }
> +  if (h->maxlen == 0)
> +    h->maxlen = -h->minblock;
> +
> +  return 0;
> +}

... but then I see you copied that code to here, so that makes sense.

> +
> +
> +static void
> +blocksize_close (void *handle)

Double blank line before this function.

The rest of it looks like the straightforward refactoring with
addition of .prepare to set up the handle, so ACK with double blank
line removed, and maybe with the globals renamed.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v