Re: [Libguestfs] [PATCH nbdkit 2/2] common/include: Fix MIN and MAX macros so they can be nested
On Tue, Feb 22, 2022 at 09:35:04PM +, Richard W.M. Jones wrote: > Thanks: Eric Blake > --- > common/include/minmax.h | 21 + > common/include/test-minmax.c | 14 ++ > 2 files changed, 27 insertions(+), 8 deletions(-) ACK. -- 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
Re: [Libguestfs] [PATCH nbdkit 1/2] common/include: Change unique-name macros to use __COUNTER__
On Tue, Feb 22, 2022 at 09:35:03PM +, Richard W.M. Jones wrote:
> Previously the macros used __LINE__ which meant they created a unique
> name specific to the line on which the macro was expanded. This
> worked to a limited degree for cases like:
>
> #define FOO \
> ({ int NBDKIT_UNIQUE_NAME(foo) = 42; \
> NBDKIT_UNIQUE_NAME(foo) * 2 })
Missing a ; after '* 2'.
>
> since the “FOO” macro is usually expanded at one place so both uses of
> NBDKIT_UNIQUE_NAME expanded to the same unique name. It didn't work
> if FOO was used twice on the same line, eg:
>
> int i = FOO * FOO;
This didn't actually fail. The failure we saw was more subtle:
MIN (MIN (1, 2), 3)
compiled (the inner MIN() creates an _x evaluated in the context of
the initializer of the outer MIN's _x, which is not in scope until the
initializer completes), but:
MIN (1, MIN (2, 3))
failed to compile with -Werror -Wshadow, because now the inner MIN's
_x is declared inside the scope of the initializer of the outer MIN's
_y, when an outer _x is already in scope.
>
> would fail, but this would work:
>
> int i = FOO *
> FOO;
Or, back to the example we actually hit,
MIN (1,
MIN (2, 3))
worked.
>
> Use __COUNTER__ instead of __LINE__, but NBDKIT_UNIQUE_NAME must now
> be used differently. The FOO macro above must be rewritten as:
>
> #define FOO FOO_1(NBDKIT_UNIQUE_NAME(foo))
> #define FOO_1(foo) \
> ({ int foo = 42; \
> foo * 2 })
>
> Thanks: Eric Blake
> ---
> +++ b/common/include/test-checked-overflow.c
> @@ -39,29 +39,25 @@
>
> #define TEST_MUL(a, b, result, expected_overflow, expected_result) \
>do { \
> -bool NBDKIT_UNIQUE_NAME(_actual_overflow); \
> +bool actual_overflow; \
> \
> -NBDKIT_UNIQUE_NAME(_actual_overflow) = \
> - MUL_OVERFLOW_FALLBACK ((a), (b), (result)); \
> -assert (NBDKIT_UNIQUE_NAME(_actual_overflow) == (expected_overflow)); \
> +actual_overflow = MUL_OVERFLOW_FALLBACK ((a), (b), (result)); \
Extra spacing after =
The commit message may need touching up, but ACK to the change itself.
--
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
Re: [Libguestfs] [PATCH nbdkit 0/2] common/include: Fix MIN and MAX macros so they can be nested
On Tue, Feb 22, 2022 at 09:35:02PM +, Richard W.M. Jones wrote: > Not tested very much yet. I need to test them across a lot more > platforms and compilers. However for a first cut they look good. I tested the patches now on: - FreeBSD - OpenBSD - clang on Linux - mingw-w64 (Fedora Windows cross compiler) and they work on all of those. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH nbdkit 2/2] common/include: Fix MIN and MAX macros so they can be nested
Thanks: Eric Blake
---
common/include/minmax.h | 21 +
common/include/test-minmax.c | 14 ++
2 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/common/include/minmax.h b/common/include/minmax.h
index d3e667ea..8d1fbf19 100644
--- a/common/include/minmax.h
+++ b/common/include/minmax.h
@@ -44,16 +44,23 @@
* is required.
*/
+#include "unique-name.h"
+
+#undef MIN
+#define MIN(x, y) \
+ MIN_1((x), (y), NBDKIT_UNIQUE_NAME(_x), NBDKIT_UNIQUE_NAME(_y))
+#undef MAX
+#define MAX(x, y) \
+ MAX_1((x), (y), NBDKIT_UNIQUE_NAME(_x), NBDKIT_UNIQUE_NAME(_y))
+
#ifdef HAVE_AUTO_TYPE
-#undef MIN
-#define MIN(x, y) ({ \
+#define MIN_1(x, y, _x, _y) ({ \
__auto_type _x = (x);\
__auto_type _y = (y);\
_x < _y ? _x : _y; \
})
-#undef MAX
-#define MAX(x, y) ({ \
+#define MAX_1(x, y, _x, _y) ({ \
__auto_type _x = (x);\
__auto_type _y = (y);\
_x > _y ? _x : _y; \
@@ -61,14 +68,12 @@
#else
-#undef MIN
-#define MIN(x, y) ({ \
+#define MIN_1(x, y, _x, _y) ({ \
typeof (x) _x = (x); \
typeof (y) _y = (y); \
_x < _y ? _x : _y; \
})
-#undef MAX
-#define MAX(x, y) ({ \
+#define MAX_1(x, y, _x, _y) ({ \
typeof (x) _x = (x); \
typeof (y) _y = (y); \
_x > _y ? _x : _y; \
diff --git a/common/include/test-minmax.c b/common/include/test-minmax.c
index 7584ab27..32bc8a11 100644
--- a/common/include/test-minmax.c
+++ b/common/include/test-minmax.c
@@ -154,5 +154,19 @@ main (void)
SIGNED_TEST (f, -FLT_MAX, FLT_MAX);
SIGNED_TEST (d, -DBL_MAX, DBL_MAX);
+ /* Test that MIN and MAX can be nested. This is really a compile
+ * test, but we do check the answer.
+ */
+ assert (MIN (MIN (1, 2), 3) == 1);
+ assert (MAX (MIN (1, 2), 3) == 3);
+ assert (MIN (MAX (1, 2), 3) == 2);
+ assert (MAX (MAX (1, 4), 3) == 4);
+ assert (MIN (3, MIN (1, 2)) == 1);
+ assert (MAX (3, MIN (1, 2)) == 3);
+ assert (MIN (3, MAX (1, 2)) == 2);
+ assert (MAX (3, MAX (1, 4)) == 4);
+ assert (MIN (MIN (1, MIN (2, 3)), 4) == 1);
+ assert (MAX (MAX (1, MAX (2, 3)), 4) == 4);
+
exit (EXIT_SUCCESS);
}
--
2.35.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH nbdkit 1/2] common/include: Change unique-name macros to use __COUNTER__
Previously the macros used __LINE__ which meant they created a unique
name specific to the line on which the macro was expanded. This
worked to a limited degree for cases like:
#define FOO \
({ int NBDKIT_UNIQUE_NAME(foo) = 42; \
NBDKIT_UNIQUE_NAME(foo) * 2 })
since the “FOO” macro is usually expanded at one place so both uses of
NBDKIT_UNIQUE_NAME expanded to the same unique name. It didn't work
if FOO was used twice on the same line, eg:
int i = FOO * FOO;
would fail, but this would work:
int i = FOO *
FOO;
Use __COUNTER__ instead of __LINE__, but NBDKIT_UNIQUE_NAME must now
be used differently. The FOO macro above must be rewritten as:
#define FOO FOO_1(NBDKIT_UNIQUE_NAME(foo))
#define FOO_1(foo) \
({ int foo = 42; \
foo * 2 })
Thanks: Eric Blake
---
common/include/checked-overflow.h | 36 --
common/include/test-checked-overflow.c | 24 +++--
common/include/unique-name.h | 10 ---
common/utils/cleanup.h | 26 ---
4 files changed, 54 insertions(+), 42 deletions(-)
diff --git a/common/include/checked-overflow.h
b/common/include/checked-overflow.h
index 4f81d4c1..546c930b 100644
--- a/common/include/checked-overflow.h
+++ b/common/include/checked-overflow.h
@@ -131,33 +131,41 @@
* that the test suite can always test the fallback.
*/
#define ADD_OVERFLOW_FALLBACK(a, b, r)\
+ ADD_OVERFLOW_FALLBACK_1 ((a), (b), (r), \
+ NBDKIT_UNIQUE_NAME (_overflow),\
+ NBDKIT_UNIQUE_NAME (_tmp))
+#define ADD_OVERFLOW_FALLBACK_1(a, b, r, overflow, tmp) \
({ \
-bool NBDKIT_UNIQUE_NAME(_overflow); \
-uintmax_t NBDKIT_UNIQUE_NAME(_tmp); \
+bool overflow;\
+uintmax_t tmp;\
\
STATIC_ASSERT_UNSIGNED_INT (a); \
STATIC_ASSERT_UNSIGNED_INT (b); \
STATIC_ASSERT_UNSIGNED_INT (*(r));\
-NBDKIT_UNIQUE_NAME(_overflow) = check_add_overflow ((a), (b), \
- (typeof (*(r)))-1, \
- _UNIQUE_NAME(_tmp)); \
-*(r) = NBDKIT_UNIQUE_NAME(_tmp); \
-NBDKIT_UNIQUE_NAME(_overflow);\
+overflow = check_add_overflow ((a), (b), \
+ (typeof (*(r)))-1, \
+ ); \
+*(r) = tmp; \
+overflow; \
})
#define MUL_OVERFLOW_FALLBACK(a, b, r)\
+ MUL_OVERFLOW_FALLBACK_1 ((a), (b), (r), \
+ NBDKIT_UNIQUE_NAME (_overflow),\
+ NBDKIT_UNIQUE_NAME (_tmp))
+#define MUL_OVERFLOW_FALLBACK_1(a, b, r, overflow, tmp) \
({ \
-bool NBDKIT_UNIQUE_NAME(_overflow); \
-uintmax_t NBDKIT_UNIQUE_NAME(_tmp); \
+bool overflow;\
+uintmax_t tmp;\
\
STATIC_ASSERT_UNSIGNED_INT (a); \
STATIC_ASSERT_UNSIGNED_INT (b); \
STATIC_ASSERT_UNSIGNED_INT (*(r));\
-NBDKIT_UNIQUE_NAME(_overflow) = check_mul_overflow ((a), (b), \
- (typeof (*(r)))-1, \
- _UNIQUE_NAME(_tmp)); \
-*(r) = NBDKIT_UNIQUE_NAME(_tmp); \
-NBDKIT_UNIQUE_NAME(_overflow);\
+overflow = check_mul_overflow ((a), (b), \
+ (typeof (*(r)))-1, \
+ ); \
+*(r) = tmp; \
+overflow; \
})
/* Assert, at compile time, that the expression "x" has some unsigned integer
diff --git
[Libguestfs] [PATCH nbdkit 0/2] common/include: Fix MIN and MAX macros so they can be nested
Not tested very much yet. I need to test them across a lot more platforms and compilers. However for a first cut they look good. Rich. ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
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; 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 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 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 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 = 0x;
> > + }
> > + 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
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; 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 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,
> L,
> L,
> L,
> +L.
> L.
>
> =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 = 0x;
> + }
> + 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,
>
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd 8/8] copy: Adaptive queue size
On Mon, Feb 21, 2022 at 5:41 PM Eric Blake wrote:
>
> On Sun, Feb 20, 2022 at 02:14:03PM +0200, Nir Soffer wrote:
> > Limit the size of the copy queue also by the number of queued bytes.
> > This allows using many concurrent small requests, required to get good
> > performance, but limiting the number of large requests that are actually
> > faster with lower concurrency.
> >
> > New --queue-size option added to control the maximum queue size. With 2
> > MiB we can have 8 256 KiB requests per connection. The default queue
> > size is 16 MiB, to match the default --requests value (64) with the
> > default --request-size (256 KiB). Testing show that using more than 16
> > 256 KiB requests with one connection do not improve anything.
>
> s/do/does/
>
> >
> > The new option will simplify limiting memory usage when using large
> > requests, like this change in virt-v2v:
> > https://github.com/libguestfs/virt-v2v/commit/c943420219fa0ee971fc228aa4d9127c5ce973f7
> >
> > I tested this change with 3 images:
> >
> > - Fedora 35 + 3g of random data - hopefully simulates a real image
> > - Fully allocated image - the special case when every read command is
> > converted to a write command.
> > - Zero image - the special case when every read command is converted to
> > a zero command.
> >
> > On 2 machines:
> >
> > - laptop: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz, 12 cpus,
> > 1.5 MiB L2 cache per 2 cpus, 12 MiB L3 cache.
> > - server: Intel(R) Xeon(R) Gold 5218R CPU @ 2.10GHz, 80 cpus,
> > 1 MiB L2 cache per cpu, 27.5 MiB L3 cache.
> >
> > In all cases, both source and destination are served by qemu-nbd, using
> > --cache=none --aio=native. Because qemu-nbd does not support MULTI_CON
>
> MULTI_CONN
>
> > for writing, we are testing a single connection when copying an to
>
> Did you mean 'copying an image to'?
Yes
>
> > qemu-nbd. I tested also copying to null: since in this case we use 4
> > connections (these tests are marked with /ro).
> >
> > Results for copying all images on all machines with nbdcopy v1.11.0 and
> > this change. "before" and "after" are average time of 10 runs.
> >
> > image machinebeforeafterqueue sizeimprovement
> > ===
> > fedora laptop 3.0442.129 2m +43%
> > fulllaptop 4.9003.136 2m +56%
> > zerolaptop 3.1472.624 2m +20%
> > ---
> > fedora server 2.3242.189 16m+6%
> > fullserver 3.5213.380 8m+4%
> > zeroserver 2.2972.338 16m-2%
> > ---
> > fedora/ro laptop 2.0401.663 1m +23%
> > fedora/ro server 1.5851.393 2m +14%
> >
> > Signed-off-by: Nir Soffer
> > ---
> > copy/main.c | 52 -
> > copy/multi-thread-copying.c | 18 +++--
> > copy/nbdcopy.h | 1 +
> > copy/nbdcopy.pod| 12 +++--
> > 4 files changed, 55 insertions(+), 28 deletions(-)
> >
>
> > static void __attribute__((noreturn))
> > usage (FILE *fp, int exitcode)
> > {
> >fprintf (fp,
> > "\n"
> > "Copy to and from an NBD server:\n"
> > "\n"
> > "nbdcopy [--allocated] [-C N|--connections=N]\n"
> > "[--destination-is-zero|--target-is-zero] [--flush]\n"
> > "[--no-extents] [-p|--progress|--progress=FD]\n"
> > -"[--request-size=N] [-R N|--requests=N] [-S N|--sparse=N]\n"
> > -"[--synchronous] [-T N|--threads=N] [-v|--verbose]\n"
> > +"[--request-size=N] [--queue-size=N] [-R N|--requests=N]\n"
>
> The options are listed in mostly alphabetic order already, so
> --queue-size before --request-size makes more sense to me.
>
> > @@ -104,33 +106,35 @@ main (int argc, char *argv[])
> > {
> >enum {
> > HELP_OPTION = CHAR_MAX + 1,
> > LONG_OPTIONS,
> > SHORT_OPTIONS,
> > ALLOCATED_OPTION,
> > DESTINATION_IS_ZERO_OPTION,
> > FLUSH_OPTION,
> > NO_EXTENTS_OPTION,
> > REQUEST_SIZE_OPTION,
> > +QUEUE_SIZE_OPTION,
>
> Likewise here.
>
> > SYNCHRONOUS_OPTION,
> >};
> >const char *short_options = "C:pR:S:T:vV";
> >const struct option long_options[] = {
> > { "help", no_argument, NULL, HELP_OPTION },
> > { "long-options", no_argument, NULL, LONG_OPTIONS },
> > { "allocated", no_argument, NULL, ALLOCATED_OPTION },
> > { "connections",required_argument, NULL, 'C' },
> > { "destination-is-zero",no_argument, NULL,
> > DESTINATION_IS_ZERO_OPTION },
> > { "flush", no_argument, NULL, FLUSH_OPTION },
> > { "no-extents",
Re: [Libguestfs] [PATCH libnbd 4/8] copy: Separate finishing a command from freeing it
On Mon, Feb 21, 2022 at 5:08 PM Eric Blake wrote:
>
> On Sun, Feb 20, 2022 at 02:13:59PM +0200, Nir Soffer wrote:
> > free_command() was abused as a completion callback. Introduce
> > finish_command() completion callback, so code that want to free a
> > command does not have to add dummy errors.
> >
> > This will make it easier to manage worker state when a command
> > completes.
> >
> > Signed-off-by: Nir Soffer
> > ---
> > copy/multi-thread-copying.c | 34 --
> > 1 file changed, 20 insertions(+), 14 deletions(-)
>
> In addition to Rich's review,
>
> >
> > static int
> > -free_command (void *vp, int *error)
> > +finished_command (void *vp, int *error)
> > {
> >struct command *command = vp;
> > - struct buffer *buffer = command->slice.buffer;
> >
> >if (*error) {
> > fprintf (stderr, "write at offset %" PRId64 " failed: %s\n",
> > command->offset, strerror (*error));
> > exit (EXIT_FAILURE);
> >}
> >
> > + free_command (command);
> > +
> > + return 1; /* auto-retires the command */
> > +}
> > +
> > +static void
> > +free_command (struct command *command)
> > +{
> > + struct buffer *buffer = command->slice.buffer;
>
> Do we want to allow 'free_command (NULL)', in which case this should
> check if command is non-NULL before initializing buffer? Doing so may
> make some other cleanup paths easier to write.
I agree it is better to behave like free(). Will improve later.
>
> But for now, all callers pass in non-NULL, so ACK either way.
>
> --
> 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
Re: [Libguestfs] [PATCH libnbd 3/8] copy: Extract create_command and create_buffer helpers
On Mon, Feb 21, 2022 at 5:03 PM Eric Blake wrote:
>
> On Sun, Feb 20, 2022 at 02:13:58PM +0200, Nir Soffer wrote:
> > Creating a new command requires lot of boilerplate that makes it harder
> > to focus on the interesting code. Extract a helpers to create a command,
> > and the command slice buffer.
> >
> > create_buffer is called only once, but the compiler is smart enough to
> > inline it, and adding it makes the code much simpler.
> >
> > This change is a refactoring except fixing perror() message for calloc()
> > failure.
> >
> > Signed-off-by: Nir Soffer
> > ---
> > copy/multi-thread-copying.c | 87 +++--
> > 1 file changed, 54 insertions(+), 33 deletions(-)
>
> >if (exts.ptr[i].zero) {
> > /* The source is zero so we can proceed directly to skipping,
> > * fast zeroing, or writing zeroes at the destination.
> > */
> > -command = calloc (1, sizeof *command);
> > -if (command == NULL) {
> > - perror ("malloc");
> > - exit (EXIT_FAILURE);
> > -}
> > -command->offset = exts.ptr[i].offset;
> > -command->slice.len = exts.ptr[i].length;
> > -command->slice.base = 0;
>
> This assignment is dead code after calloc,...
>
> > -command->index = index;
> > +command = create_command (exts.ptr[i].offset, exts.ptr[i].length,
> > + true, index);
> > fill_dst_range_with_zeroes (command);
> >}
> >
> >else /* data */ {
> > /* As the extent might be larger than permitted for a single
> > * command, we may have to split this into multiple read
> > * requests.
> > */
> > while (exts.ptr[i].length > 0) {
> >len = exts.ptr[i].length;
> >if (len > request_size)
> > len = request_size;
> > - data = malloc (len);
> > - if (data == NULL) {
> > -perror ("malloc");
> > -exit (EXIT_FAILURE);
> > - }
> > - buffer = calloc (1, sizeof *buffer);
> > - if (buffer == NULL) {
> > -perror ("malloc");
> > -exit (EXIT_FAILURE);
> > - }
> > - buffer->data = data;
> > - buffer->refs = 1;
> > - command = calloc (1, sizeof *command);
> > - if (command == NULL) {
> > -perror ("malloc");
> > -exit (EXIT_FAILURE);
> > - }
> > - command->offset = exts.ptr[i].offset;
> > - command->slice.len = len;
> > - command->slice.base = 0;
>
> ...as was this,...
>
> > - command->slice.buffer = buffer;
> > - command->index = index;
> > +
> > + command = create_command (exts.ptr[i].offset, len,
> > +false, index);
> >
> >wait_for_request_slots (index);
> >
> >/* Begin the asynch read operation. */
> >src->ops->asynch_read (src, command,
> > (nbd_completion_callback) {
> > .callback = finished_read,
> > .user_data = command,
> > });
> >
> > @@ -331,20 +305,67 @@ poll_both_ends (uintptr_t index)
> > else if ((fds[1].revents & POLLOUT) != 0)
> >dst->ops->asynch_notify_write (dst, index);
> > else if ((fds[1].revents & (POLLERR | POLLNVAL)) != 0) {
> >errno = ENOTCONN;
> >perror (dst->name);
> >exit (EXIT_FAILURE);
> > }
> >}
> > }
> >
> > +/* Create a new buffer. */
> > +static struct buffer*
> > +create_buffer (size_t len)
> > +{
> > + struct buffer *buffer;
> > +
> > + buffer = calloc (1, sizeof *buffer);
> > + if (buffer == NULL) {
> > +perror ("calloc");
> > +exit (EXIT_FAILURE);
> > + }
> > +
> > + buffer->data = malloc (len);
> > + if (buffer->data == NULL) {
> > +perror ("malloc");
> > +exit (EXIT_FAILURE);
> > + }
> > +
> > + buffer->refs = 1;
> > +
> > + return buffer;
> > +}
> > +
> > +/* Create a new command for read or zero. */
> > +static struct command *
> > +create_command (uint64_t offset, size_t len, bool zero, uintptr_t index)
> > +{
> > + struct command *command;
> > +
> > + command = calloc (1, sizeof *command);
> > + if (command == NULL) {
> > +perror ("calloc");
> > +exit (EXIT_FAILURE);
> > + }
> > +
> > + command->offset = offset;
> > + command->slice.len = len;
> > + command->slice.base = 0;
>
> ...but keeping it here for the sake of refactoring is fine
Right, I will improve this later.
>
> > +
> > + if (!zero)
> > +command->slice.buffer = create_buffer (len);
> > +
> > + command->index = index;
> > +
> > + return command;
> > +}
>
> ACK
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
>
___
Re: [Libguestfs] [PATCH libnbd 2/8] copy: Rename copy_subcommand to create_subcommand
On Mon, Feb 21, 2022 at 4:52 PM Eric Blake wrote:
>
> On Sun, Feb 20, 2022 at 02:13:57PM +0200, Nir Soffer wrote:
> > copy_subcommand creates a new command without copying the original
> > command. Rename the function to make this more clear.
> >
> > Signed-off-by: Nir Soffer
> > ---
> > copy/multi-thread-copying.c | 29 ++---
> > 1 file changed, 14 insertions(+), 15 deletions(-)
> >
> > if (!last_is_zero) {
> >/* Write the last data (if any). */
> >if (i - last_offset > 0) {
> > -newcommand = copy_subcommand (command,
> > +newcommand = create_subcommand (command,
> >last_offset, i - last_offset,
> >false);
>
> Indentation needs updates here.
Will fix before pushing.
>
> > dst->ops->asynch_write (dst, newcommand,
> > (nbd_completion_callback) {
> >.callback = free_command,
> >.user_data = newcommand,
> > });
> >}
> >/* Start the new zero range. */
> >last_offset = i;
> > @@ -431,55 +430,55 @@ finished_read (void *vp, int *error)
> > }
> >}
> >else {
> > /* It's data. If the last was data too, do nothing =>
> > * coalesce. Otherwise write the last zero range and start a
> > * new data.
> > */
> > if (last_is_zero) {
> >/* Write the last zero range (if any). */
> >if (i - last_offset > 0) {
> > -newcommand = copy_subcommand (command,
> > - last_offset, i - last_offset,
> > - true);
> > +newcommand = create_subcommand (command,
> > +last_offset, i - last_offset,
> > +true);
>
> But you got it right elsewhere.
>
> ACK.
>
> --
> 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
Re: [Libguestfs] [PATCH libnbd 1/8] copy: Remove wrong references to holes
On Mon, Feb 21, 2022 at 4:51 PM Eric Blake wrote:
>
> On Sun, Feb 20, 2022 at 02:13:56PM +0200, Nir Soffer wrote:
> > In the past nbdcopy was looking for hole extents instead of zero
> > extents. When we fixed this, we forgot to update some comments and
> > variable names referencing hole instead of zero.
>
> Might be nice to add:
>
> Fixes: d5f65e36 ("copy: Do not use trim for zeroing", v1.7.3)
>
> or whatever commit you think would be better.
I will add the relevant commit before pushing.
>
> >
> > Signed-off-by: Nir Soffer
> > ---
> > copy/multi-thread-copying.c | 34 +-
> > 1 file changed, 17 insertions(+), 17 deletions(-)
> >
>
> ACK.
>
> --
> 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
Re: [Libguestfs] how can I run a subset of the tests?
On Tue, Feb 22, 2022 at 11:14:18AM +0100, Laszlo Ersek wrote:
> On 02/21/22 16:56, Richard W.M. Jones wrote:
> > On Mon, Feb 21, 2022 at 03:10:02PM +0100, Laszlo Ersek wrote:
> >> Hi,
> >>
> >> "make check" in libguestfs takes very long (especially when it's run
> >> after every patch in a series).
> >>
> >> How can I run only those tests that are, for example, in "tests/luks/"?
> >
> > This used to be possible before:
> >
> > commit 6d32773e811882f78dbd8c2a39a2b7a9c3cfca7c
> > Author: Richard W.M. Jones
> > Date: Thu Mar 18 11:15:06 2021 +
> >
> > tests: Run the tests in parallel.
> >
> > As far as I know it's no longer possible to run just tests from a
> > single directory.
> >
> > However - a bit clumsy - if you know the exact list of tests you want
> > to run then this works:
> >
> > $ make -C tests check TESTS=" luks/test-luks.sh luks/test-luks-list.sh
> > luks/test-key-option.sh luks/test-key-option-inspect.sh "
>
> Sigh, I'd actually tried something like this, based on a
> stackoverflow.com hint; however, there is so much cruft on that command
> line (options with arguments: "-C tests", and in my case: "-j 10"; an
I use:
$ grep MAKEFLAGS ~/.bash_profile
export MAKEFLAGS=-j`nproc`
$ echo $MAKEFLAGS
-j24
(You'd be surprised how many things are broken by this, eg it's a rich
source of bugs in random RPM builds that don't expect to inherit
$MAKEFLAGS from the environment combined with having incomplete
dependencies.)
Rich.
> operand that is a target: "check", and an operand that is a macro
> definition: "TESTS=...") that (from my bash history) it looks like I
> left out the target ("check")...
>
> Thanks!
> Laszlo
>
> > ...
> > make[3]: Entering directory '/home/rjones/d/libguestfs/tests'
> > SKIP: luks/test-key-option-inspect.sh
> > PASS: luks/test-luks-list.sh
> > PASS: luks/test-key-option.sh
> > PASS: luks/test-luks.sh
> >
> > Testsuite summary for libguestfs 1.47.2
> >
> > # TOTAL: 4
> > # PASS: 3
> > # SKIP: 1
> > # XFAIL: 0
> > # FAIL: 0
> > # XPASS: 0
> > # ERROR: 0
> >
> > make[3]: Leaving directory '/home/rjones/d/libguestfs/tests'
> > make[2]: Leaving directory '/home/rjones/d/libguestfs/tests'
> > make[1]: Leaving directory '/home/rjones/d/libguestfs/tests'
> > make: Leaving directory '/home/rjones/d/libguestfs/tests'
> >
> > Rich.
> >
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] how can I run a subset of the tests?
On 02/21/22 16:56, Richard W.M. Jones wrote:
> On Mon, Feb 21, 2022 at 03:10:02PM +0100, Laszlo Ersek wrote:
>> Hi,
>>
>> "make check" in libguestfs takes very long (especially when it's run
>> after every patch in a series).
>>
>> How can I run only those tests that are, for example, in "tests/luks/"?
>
> This used to be possible before:
>
> commit 6d32773e811882f78dbd8c2a39a2b7a9c3cfca7c
> Author: Richard W.M. Jones
> Date: Thu Mar 18 11:15:06 2021 +
>
> tests: Run the tests in parallel.
>
> As far as I know it's no longer possible to run just tests from a
> single directory.
>
> However - a bit clumsy - if you know the exact list of tests you want
> to run then this works:
>
> $ make -C tests check TESTS=" luks/test-luks.sh luks/test-luks-list.sh
> luks/test-key-option.sh luks/test-key-option-inspect.sh "
Sigh, I'd actually tried something like this, based on a
stackoverflow.com hint; however, there is so much cruft on that command
line (options with arguments: "-C tests", and in my case: "-j 10"; an
operand that is a target: "check", and an operand that is a macro
definition: "TESTS=...") that (from my bash history) it looks like I
left out the target ("check")...
Thanks!
Laszlo
> ...
> make[3]: Entering directory '/home/rjones/d/libguestfs/tests'
> SKIP: luks/test-key-option-inspect.sh
> PASS: luks/test-luks-list.sh
> PASS: luks/test-key-option.sh
> PASS: luks/test-luks.sh
>
> Testsuite summary for libguestfs 1.47.2
>
> # TOTAL: 4
> # PASS: 3
> # SKIP: 1
> # XFAIL: 0
> # FAIL: 0
> # XPASS: 0
> # ERROR: 0
>
> make[3]: Leaving directory '/home/rjones/d/libguestfs/tests'
> make[2]: Leaving directory '/home/rjones/d/libguestfs/tests'
> make[1]: Leaving directory '/home/rjones/d/libguestfs/tests'
> make: Leaving directory '/home/rjones/d/libguestfs/tests'
>
> Rich.
>
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] cache, cow: Export block size constraints
On 02/21/22 15:32, Richard W.M. Jones wrote:
>
> On Mon, Feb 21, 2022 at 03:19:23PM +0100, Laszlo Ersek wrote:
>> On 02/21/22 11:22, Richard W.M. Jones wrote:
>>> On Mon, Feb 21, 2022 at 10:22:04AM +0100, Laszlo Ersek wrote:
> +/* Block size constraints. */
> +static int
> +cache_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 = 1;
> +*preferred = blksize;
> +*maximum = 0x;
> + }
> + else if (*maximum >= blksize) {
Do we need braces here?
> +*preferred = MAX (*preferred, blksize);
> + }
>>>
>>> I don't think we need them, but it might be clearer with them.
>>
>> Sorry, I'm still a bit confused whether braces around single statements
>> are *permitted* -- the rule even seems to vary across the various v2v
>> projects.
>
> Oh I got the wrong end of the stick. I thought you meant would the
> code be clearer like this:
>
> + if (*minimum == 0) { /* No constraints set by the plugin. */
> +*minimum = 1;
> +*preferred = blksize;
> +*maximum = 0x;
> + }
> + else {
> +if (*maximum >= blksize)
> + *preferred = MAX (*preferred, blksize);
> + }
>
> I think it would be clearer, since the else clause now refers to the
> separate case where the plugin *did* set constraints. In fact I
> changed my local copy already.
>
> But I think you meant should I have used braces around the single line
> statement (in the original code).
>
>> My understanding has been that we forbid braces around single
>> statements, at least in some projects. So what's the rule?
>
> There's not really a rule. I tend to use whatever is clearer in the
> particular case. ie. Trading off code density (being able to see more
> code on the page helps me) vs clarity (braces can be used to make it
> clearer what code is contained in a clause, but correct indentation
> does that as well).
Thanks! I've not met a project yet that wasn't very strict about braces
related to if/else. E.g. QEMU and edk2 require braces in all cases;
while Linux forbids braces around single statements (unless the proper
meaning of the code requires them).
I'm OK (of course!) with "whatever feels best on the spot".
Personally I've grown to "use braces everywhere", because (a) it's
easier to insert new stuff then, (b) my editor supports jumping from/to,
and highlighting, matching braces -- but that requires braces. :)
Thanks!
Laszlo
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
