[Libguestfs] [nbdkit PATCH] swab: Implement .block_size callback

Laszlo Ersek lersek at redhat.com
Tue Feb 22 11:49:13 UTC 2022


On 02/21/22 23:00, Eric Blake wrote:
> We were previously enforcing minimum block size with EINVAL for
> too-small requests.  Advertise this to the client.
> ---
>  filters/swab/nbdkit-swab-filter.pod |  6 ++++++
>  filters/swab/swab.c                 | 24 +++++++++++++++++++++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/filters/swab/nbdkit-swab-filter.pod b/filters/swab/nbdkit-swab-filter.pod
> index f8500150..030a0852 100644
> --- a/filters/swab/nbdkit-swab-filter.pod
> +++ b/filters/swab/nbdkit-swab-filter.pod
> @@ -34,6 +34,11 @@ the last few bytes, combine this filter with
>  L<nbdkit-truncate-filter(1)>; fortunately, sector-based disk images
>  are already suitably sized.
> 
> +Note that this filter fails operations that are not aligned to the
> +swab-bits boundaries; if you need byte-level access, apply the
> +L<nbdkit-blocksize-filter(1)> before this one, to get
> +read-modify-write access to individual bytes.
> +
>  =head1 PARAMETERS

I understand that the alignment of requests is enforced, but what
happens if the client sends a request (correctly aligned) that is 17
bytes in size, for example?

... Aha, so is_aligned() doesn't only check "offset", it also checks
"count". That wasn't clear to me from the addition to
"filters/swab/nbdkit-swab-filter.pod". I suggest spelling that out more
explicitly.

> 
>  =over 4
> @@ -90,6 +95,7 @@ L<nbdkit(1)>,
>  L<nbdkit-file-plugin(1)>,
>  L<nbdkit-pattern-plugin(1)>,
>  L<nbdkit-filter(3)>,
> +L<nbdkit-blocksize-filter(1)>.
>  L<nbdkit-truncate-filter(1)>.
> 
>  =head1 AUTHORS
> diff --git a/filters/swab/swab.c b/filters/swab/swab.c
> index 68776eee..6e8dc981 100644
> --- a/filters/swab/swab.c
> +++ b/filters/swab/swab.c
> @@ -44,6 +44,7 @@
>  #include "byte-swapping.h"
>  #include "isaligned.h"
>  #include "cleanup.h"
> +#include "minmax.h"
>  #include "rounding.h"
> 
>  /* Can only be 8 (filter disabled), 16, 32 or 64. */
> @@ -85,8 +86,28 @@ swab_get_size (nbdkit_next *next,
>    return ROUND_DOWN (size, bits/8);
>  }
> 
> +/* Block size constraints. */
> +static int
> +swab_block_size (nbdkit_next *next, void *handle,
> +                 uint32_t *minimum, uint32_t *preferred, uint32_t *maximum)
> +{
> +  if (next->block_size (next, minimum, preferred, maximum) == -1)
> +    return -1;
> +
> +  if (*minimum == 0) {         /* No constraints set by the plugin. */
> +    *minimum = bits/8;
> +    *preferred = 512;
> +    *maximum = 0xffffffff;
> +  }
> +  else {
> +    *minimum = MAX (*minimum, bits/8);
> +  }

Given that the count too must be a whole multiple of the swap-block size
(correctly so), what if the underlying plugin specifies a minimum block
size of 17?

I think that will take effect here, and then this filter will specify
such a minimum block size (17) that it will, in turn, reject
unconditionally. That kind of defeats the purpose of exposing a "minimum
block size".

Wouldn't it be better if, on the "else" branch, we rounded up "*minimum"?

  *minimum = ROUND_UP (*minimum, bits/8);

Thanks,
Laszlo


> +
> +  return 0;
> +}
> +
>  /* The request must be aligned.
> - * XXX We could lift this restriction with more work.
> + * If you want finer alignment, use the blocksize filter.
>   */
>  static bool
>  is_aligned (uint32_t count, uint64_t offset, int *err)
> @@ -220,6 +241,7 @@ static struct nbdkit_filter filter = {
>    .config            = swab_config,
>    .config_help       = swab_config_help,
>    .get_size          = swab_get_size,
> +  .block_size        = swab_block_size,
>    .pread             = swab_pread,
>    .pwrite            = swab_pwrite,
>    .trim              = swab_trim,
> 




More information about the Libguestfs mailing list