[nbdkit PATCH] swab: Implement .block_size callback
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 =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); + } + + 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, -- 2.35.1
On Mon, Feb 21, 2022 at 04:00:11PM -0600, 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
=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); + } + + 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,
Looks sensible, ACK Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
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,
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
On 02/22/22 15:41, Eric Blake wrote:
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
Thanks!
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones