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
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
