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

On Tue, Feb 22, 2022 at 12:49:13PM +0100, Laszlo Ersek wrote: > 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. I went with: -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. +Note that this filter fails operations where the offset or count 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. >> +/* 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? Not possible ;) Minimum block size must be a power of 2 between 1 and 64k; the plugin layer enforces this. Since swab-bits alignments are 1, 2, 4, or 8 (also a power of 2), the MAX() operation is sufficient without needing ROUND_UP. > > 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); > Now in as amended as commit b9f8ef83