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

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.

> +/* 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);