[Libguestfs] [PATCH nbdkit 2/3] sh: Switch nbdkit-sh-plugin to use API version 2.

[Eric Blake]

On 12/14/18 4:16 PM, Richard W.M. Jones wrote:
> This allows us to add the following callbacks:
> 
>   - can_zero     returns boolean
>   - can_fua      should print "none", "emulate" or "native"
> 
> Furthermore the following callbacks are modified in a backwards
> compatible manner:
> 
>   - pwrite       adding flags parameter
>   - trim         adding flags parameter
> 
> This change is not backwards compatible:
> 
>   - zero         may_trim parameter replaced by flags parameter
> ---
>   plugins/sh/nbdkit-sh-plugin.pod | 41 +++++++++++---
>   plugins/sh/sh.c                 | 99 +++++++++++++++++++++++++++------
>   2 files changed, 115 insertions(+), 25 deletions(-)
> 

>   
> +=item C<can_fua>
> +
> + /path/to/script can_fua <handle>
> +
> +This controls Forced Unit Access (FUA) behaviour of the core server.
> +Unlike the other C<can_*> callbacks, this one is I<not> a boolean.  It
> +must print either "none", "emulate" or "native".  The meaning of these

Worth adding 'to stdout'?

> +is described in L<nbdkit-plugin(3)>.
> +
>   =item C<pread>
>   
>    /path/to/script pread <handle> <count> <offset>
> @@ -227,10 +240,13 @@ This method is required.
>   
>   =item C<pwrite>
>   
> - /path/to/script pwrite <handle> <count> <offset>
> + /path/to/script pwrite <handle> <count> <offset> <flags>
>   
>   The script should read the binary data to be written from stdin.
>   
> +The C<flags> parameter can be an empty string or C<"fua">.  In future

s/In future/In the future,/

> +a comma-separated list of flags may be present.

Is the empty string "" always provided as an argument, or will the 
argument be omitted in that case?

> +
>   Unlike in other languages, if you provide a C<pwrite> method you
>   B<must> also provide a C<can_write> method which exits with code C<0>
>   (true).
> @@ -245,16 +261,23 @@ B<must> also provide a C<can_flush> method which exits with code C<0>
>   
>   =item C<trim>
>   
> - /path/to/script trim <handle> <count> <offset>
> + /path/to/script trim <handle> <count> <offset> <flags>
> +
> +The C<flags> parameter can be an empty string or C<"fua">.  In future

Copy-and-paste "In future"

> +a comma-separated list of flags may be present.
>   
>   Unlike in other languages, if you provide a C<trim> method you B<must>
>   also provide a C<can_trim> method which exits with code C<0> (true).
>   
>   =item C<zero>
>   
> - /path/to/script zero <handle> <count> <offset> <may_trim>
> + /path/to/script zero <handle> <count> <offset> <flags>
>   
> -C<may_trim> will be either C<true> or C<false>.
> +The C<flags> parameter can be an empty string or C<"fua">.  In future
> +a comma-separated list of flags may be present.

This one is wrong, since you have both fua and may_trim now.

> +
> +  case RET_FALSE:
> +    free (s);
> +    nbdkit_error ("%s: %s method returned unexpected code (3/false)",
> +                  script, "can_fua");

Could we be nice and treat this as "none"?

> @@ -538,12 +602,13 @@ sh_trim (void *handle, uint32_t count, uint64_t offset)
>   }
>   
>   static int
> -sh_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)
> +sh_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
>   {
>     char *h = handle;
>     char cbuf[32], obuf[32];
> -  const char *args[] = { script, "zero", h, cbuf, obuf,
> -                         may_trim ? "true" : "false", NULL };
> +  /* In future, comma-separated list of flags. */

While I commented about 'in future' being too terse in the .pod, I don't 
mind it being compressed in the .c file.   On the other hand, the 
comment here is wrong, as sh_zero needs to handle both fua and may_trim 
flags.

> +  const char *sflags = flags & NBDKIT_FLAG_FUA ? "fua" : "";

and thus this computation for sflags is incomplete.

Otherwise, looking good.

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