10:36 p.m.
Either patch 1 or patch 2 in isolation is sufficient to fix the
problem that Rich forwarded on from an archlinux tester (name so I can
credit them?). But both patches should be applied, as well as
backported to appropriate stable branches, to maximize cross-version
interoperability of nbdkit vs. plugins. Patch 3 will let us detect
future similar bugs much faster. I want patch 4 to ensure that we
guarantee our API in the future, but it isn't finished yet.
Then I still need to follow up with my threat to remove nbd-standalone
for 1.20 (relying on libnbd is a lot easier now than it was when we
first added optional libnbd support in 1.14).
Eric Blake (4):
server: Normalize plugin can_* values
nbd: Normalize return values of can_*
tests: Don't let test-parallel-* hang on nbdkit bug
RFC tests: Add test to cover unusual .can_flush return
tests/Makefile.am | 21 +++++++
server/plugins.c | 24 ++++---
plugins/nbd/nbd-standalone.c | 12 ++--
tests/test-flush.sh | 85 +++++++++++++++++++++++++
tests/test-parallel-file.sh | 19 +++---
tests/test-parallel-nbd.sh | 13 ++--
tests/test-parallel-sh.sh | 19 +++---
tests/test-flush-plugin.c | 119 +++++++++++++++++++++++++++++++++++
8 files changed, 275 insertions(+), 37 deletions(-)
create mode 100755 tests/test-flush.sh
create mode 100644 tests/test-flush-plugin.c
--
2.25.1
10:36 p.m.
New subject: [nbdkit PATCH 1/4] server: Normalize plugin can_* values
The documentation for .can_write and friends mentioned merely that the
callback must return a boolean value, or -1 on failure. Historically,
we have treated any non-zero value other than -1 as true, and some
plugins have sloppily relied on this behavior, including the
standalone nbd plugin prior to the use of libnbd in v1.14. But in
1.15.1, we added an assert in our backend code if an enabled feature
bit is not exactly 1. To avoid tripping the assertion, we have to
normalize the plugin values before storing our internal value.
Fixes: c306fa93ab
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
server/plugins.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/server/plugins.c b/server/plugins.c
index 78ed6723..4a4fcdee 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -308,13 +308,21 @@ plugin_get_size (struct backend *b, void *handle)
return p->plugin.get_size (handle);
}
+static int normalize_bool (int value)
+{
+ if (value == -1 || value == 0)
+ return value;
+ /* Normalize all other non-zero values to true */
+ return 1;
+}
+
static int
plugin_can_write (struct backend *b, void *handle)
{
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_write)
- return p->plugin.can_write (handle);
+ return normalize_bool (p->plugin.can_write (handle));
else
return p->plugin.pwrite || p->plugin._pwrite_v1;
}
@@ -325,7 +333,7 @@ plugin_can_flush (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_flush)
- return p->plugin.can_flush (handle);
+ return normalize_bool (p->plugin.can_flush (handle));
else
return p->plugin.flush || p->plugin._flush_v1;
}
@@ -336,7 +344,7 @@ plugin_is_rotational (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.is_rotational)
- return p->plugin.is_rotational (handle);
+ return normalize_bool (p->plugin.is_rotational (handle));
else
return 0; /* assume false */
}
@@ -347,7 +355,7 @@ plugin_can_trim (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_trim)
- return p->plugin.can_trim (handle);
+ return normalize_bool (p->plugin.can_trim (handle));
else
return p->plugin.trim || p->plugin._trim_v1;
}
@@ -380,7 +388,7 @@ plugin_can_fast_zero (struct backend *b, void *handle)
int r;
if (p->plugin.can_fast_zero)
- return p->plugin.can_fast_zero (handle);
+ return normalize_bool (p->plugin.can_fast_zero (handle));
/* Advertise support for fast zeroes if no .zero or .can_zero is
* false: in those cases, we fail fast instead of using .pwrite.
* This also works when v1 plugin has only ._zero_v1.
@@ -399,7 +407,7 @@ plugin_can_extents (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_extents)
- return p->plugin.can_extents (handle);
+ return normalize_bool (p->plugin.can_extents (handle));
else
return p->plugin.extents != NULL;
}
@@ -430,7 +438,7 @@ plugin_can_multi_conn (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_multi_conn)
- return p->plugin.can_multi_conn (handle);
+ return normalize_bool (p->plugin.can_multi_conn (handle));
else
return 0; /* assume false */
}
--
2.25.1
3:10 a.m.
New subject: [nbdkit PATCH 1/4] server: Normalize plugin can_* values
On Mon, Mar 16, 2020 at 10:36:14PM -0500, Eric Blake wrote:
The documentation for .can_write and friends mentioned merely that
the
callback must return a boolean value, or -1 on failure. Historically,
we have treated any non-zero value other than -1 as true, and some
plugins have sloppily relied on this behavior, including the
standalone nbd plugin prior to the use of libnbd in v1.14. But in
1.15.1, we added an assert in our backend code if an enabled feature
bit is not exactly 1. To avoid tripping the assertion, we have to
normalize the plugin values before storing our internal value.
Fixes: c306fa93ab
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
server/plugins.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/server/plugins.c b/server/plugins.c
index 78ed6723..4a4fcdee 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -308,13 +308,21 @@ plugin_get_size (struct backend *b, void *handle)
return p->plugin.get_size (handle);
}
+static int normalize_bool (int value)
+{
+ if (value == -1 || value == 0)
+ return value;
+ /* Normalize all other non-zero values to true */
+ return 1;
+}
+
static int
plugin_can_write (struct backend *b, void *handle)
{
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_write)
- return p->plugin.can_write (handle);
+ return normalize_bool (p->plugin.can_write (handle));
else
return p->plugin.pwrite || p->plugin._pwrite_v1;
}
@@ -325,7 +333,7 @@ plugin_can_flush (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_flush)
- return p->plugin.can_flush (handle);
+ return normalize_bool (p->plugin.can_flush (handle));
else
return p->plugin.flush || p->plugin._flush_v1;
}
@@ -336,7 +344,7 @@ plugin_is_rotational (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.is_rotational)
- return p->plugin.is_rotational (handle);
+ return normalize_bool (p->plugin.is_rotational (handle));
else
return 0; /* assume false */
}
@@ -347,7 +355,7 @@ plugin_can_trim (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_trim)
- return p->plugin.can_trim (handle);
+ return normalize_bool (p->plugin.can_trim (handle));
else
return p->plugin.trim || p->plugin._trim_v1;
}
@@ -380,7 +388,7 @@ plugin_can_fast_zero (struct backend *b, void *handle)
int r;
if (p->plugin.can_fast_zero)
- return p->plugin.can_fast_zero (handle);
+ return normalize_bool (p->plugin.can_fast_zero (handle));
/* Advertise support for fast zeroes if no .zero or .can_zero is
* false: in those cases, we fail fast instead of using .pwrite.
* This also works when v1 plugin has only ._zero_v1.
@@ -399,7 +407,7 @@ plugin_can_extents (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_extents)
- return p->plugin.can_extents (handle);
+ return normalize_bool (p->plugin.can_extents (handle));
else
return p->plugin.extents != NULL;
}
@@ -430,7 +438,7 @@ plugin_can_multi_conn (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_multi_conn)
- return p->plugin.can_multi_conn (handle);
+ return normalize_bool (p->plugin.can_multi_conn (handle));
else
return 0; /* assume false */
}
--
Nice one, ACK.
I guess this one will need to be backported to (only?) 1.16 and 1.18?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
6:55 a.m.
New subject: [nbdkit PATCH 1/4] server: Normalize plugin can_* values
On 3/17/20 3:10 AM, Richard W.M. Jones wrote:
On Mon, Mar 16, 2020 at 10:36:14PM -0500, Eric Blake wrote:
> The documentation for .can_write and friends mentioned merely that the
> callback must return a boolean value, or -1 on failure. Historically,
> we have treated any non-zero value other than -1 as true, and some
> plugins have sloppily relied on this behavior, including the
> standalone nbd plugin prior to the use of libnbd in v1.14. But in
> 1.15.1, we added an assert in our backend code if an enabled feature
> bit is not exactly 1. To avoid tripping the assertion, we have to
> normalize the plugin values before storing our internal value.
>
> Fixes: c306fa93ab
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
Nice one, ACK.
I guess this one will need to be backported to (only?) 1.16 and 1.18?
Those are the only two branches with the assertion failure. Backporting
to earlier versions is optional (it solves a latent bug, but unless we
plan on backporting other patches that would tickle the latent bug, it
seems like churn).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
10:36 p.m.
New subject: [nbdkit PATCH 2/4] nbd: Normalize return values of can_*
Although nbdkit documents that any positive value should be treated as
success to the .can_* callbacks, we had a window of releases where
anything other than 1 could trigger an assertion failure, fixed in the
previous patch. Update what we return to avoid tripping the bug in
broken nbdkit.
Our return value has been latent since 0e8e8eb11d (v1.1.17), but the
assertion failures did not occur until c306fa93ab and neighboring
commits (v1.15.1). As v1.13.6 was the point where we started
preferring builds against libnbd, where we always returned 1 on
success, the problem was not detected until now; but it is still in
the wild for any user mixing old plugins with new libnbd.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/nbd/nbd-standalone.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/plugins/nbd/nbd-standalone.c b/plugins/nbd/nbd-standalone.c
index cbf9dae7..4230ecd9 100644
--- a/plugins/nbd/nbd-standalone.c
+++ b/plugins/nbd/nbd-standalone.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2017-2019 Red Hat Inc.
+ * Copyright (C) 2017-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -1175,7 +1175,7 @@ nbd_can_flush (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_SEND_FLUSH;
+ return !!(h->flags & NBD_FLAG_SEND_FLUSH);
}
static int
@@ -1183,7 +1183,7 @@ nbd_is_rotational (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_ROTATIONAL;
+ return !!(h->flags & NBD_FLAG_ROTATIONAL);
}
static int
@@ -1191,7 +1191,7 @@ nbd_can_trim (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_SEND_TRIM;
+ return !!(h->flags & NBD_FLAG_SEND_TRIM);
}
static int
@@ -1199,7 +1199,7 @@ nbd_can_zero (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_SEND_WRITE_ZEROES;
+ return !!(h->flags & NBD_FLAG_SEND_WRITE_ZEROES);
}
static int
@@ -1215,7 +1215,7 @@ nbd_can_multi_conn (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_CAN_MULTI_CONN;
+ return !!(h->flags & NBD_FLAG_CAN_MULTI_CONN);
}
static int
--
2.25.1
3:12 a.m.
New subject: [nbdkit PATCH 2/4] nbd: Normalize return values of can_*
On Mon, Mar 16, 2020 at 10:36:15PM -0500, Eric Blake wrote:
Although nbdkit documents that any positive value should be treated
as
success to the .can_* callbacks, we had a window of releases where
anything other than 1 could trigger an assertion failure, fixed in the
previous patch. Update what we return to avoid tripping the bug in
broken nbdkit.
Our return value has been latent since 0e8e8eb11d (v1.1.17), but the
assertion failures did not occur until c306fa93ab and neighboring
commits (v1.15.1). As v1.13.6 was the point where we started
preferring builds against libnbd, where we always returned 1 on
success, the problem was not detected until now; but it is still in
the wild for any user mixing old plugins with new libnbd.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/nbd/nbd-standalone.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/plugins/nbd/nbd-standalone.c b/plugins/nbd/nbd-standalone.c
index cbf9dae7..4230ecd9 100644
--- a/plugins/nbd/nbd-standalone.c
+++ b/plugins/nbd/nbd-standalone.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2017-2019 Red Hat Inc.
+ * Copyright (C) 2017-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -1175,7 +1175,7 @@ nbd_can_flush (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_SEND_FLUSH;
+ return !!(h->flags & NBD_FLAG_SEND_FLUSH);
}
static int
@@ -1183,7 +1183,7 @@ nbd_is_rotational (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_ROTATIONAL;
+ return !!(h->flags & NBD_FLAG_ROTATIONAL);
}
static int
@@ -1191,7 +1191,7 @@ nbd_can_trim (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_SEND_TRIM;
+ return !!(h->flags & NBD_FLAG_SEND_TRIM);
}
static int
@@ -1199,7 +1199,7 @@ nbd_can_zero (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_SEND_WRITE_ZEROES;
+ return !!(h->flags & NBD_FLAG_SEND_WRITE_ZEROES);
}
static int
@@ -1215,7 +1215,7 @@ nbd_can_multi_conn (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_CAN_MULTI_CONN;
+ return !!(h->flags & NBD_FLAG_CAN_MULTI_CONN);
}
static int
While I don't like the idea of modifying plugins to fix broken nbdkit
behaviour, you make a good case for this being safer, so:
ACK
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
6:59 a.m.
New subject: [nbdkit PATCH 2/4] nbd: Normalize return values of can_*
On 3/17/20 3:12 AM, Richard W.M. Jones wrote:
On Mon, Mar 16, 2020 at 10:36:15PM -0500, Eric Blake wrote:
> Although nbdkit documents that any positive value should be treated as
> success to the .can_* callbacks, we had a window of releases where
> anything other than 1 could trigger an assertion failure, fixed in the
> previous patch. Update what we return to avoid tripping the bug in
> broken nbdkit.
>
> Our return value has been latent since 0e8e8eb11d (v1.1.17), but the
> assertion failures did not occur until c306fa93ab and neighboring
> commits (v1.15.1). As v1.13.6 was the point where we started
> preferring builds against libnbd, where we always returned 1 on
> success, the problem was not detected until now; but it is still in
> the wild for any user mixing old plugins with new libnbd.
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
> plugins/nbd/nbd-standalone.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
While I don't like the idea of modifying plugins to fix broken
nbdkit
behaviour, you make a good case for this being safer, so:
ACK
Think of it as working around broken nbdkit behavior, rather than fixing
it (as the fix is in patch 1/4). How many stable branches should we
backport this to? Going all the way back to stable-1.2 seems like it
might be overkill; maybe back to stable-1.14 (where nbd-standalone was
first split off) or stable-1.12 (the last release that did not use
libnbd) is sufficient?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
7:10 a.m.
New subject: [nbdkit PATCH 2/4] nbd: Normalize return values of can_*
On Tue, Mar 17, 2020 at 06:59:09AM -0500, Eric Blake wrote:
On 3/17/20 3:12 AM, Richard W.M. Jones wrote:
>On Mon, Mar 16, 2020 at 10:36:15PM -0500, Eric Blake wrote:
>>Although nbdkit documents that any positive value should be treated as
>>success to the .can_* callbacks, we had a window of releases where
>>anything other than 1 could trigger an assertion failure, fixed in the
>>previous patch. Update what we return to avoid tripping the bug in
>>broken nbdkit.
>>
>>Our return value has been latent since 0e8e8eb11d (v1.1.17), but the
>>assertion failures did not occur until c306fa93ab and neighboring
>>commits (v1.15.1). As v1.13.6 was the point where we started
>>preferring builds against libnbd, where we always returned 1 on
>>success, the problem was not detected until now; but it is still in
>>the wild for any user mixing old plugins with new libnbd.
>>
>>Signed-off-by: Eric Blake <eblake(a)redhat.com>
>>---
>> plugins/nbd/nbd-standalone.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>While I don't like the idea of modifying plugins to fix broken nbdkit
>behaviour, you make a good case for this being safer, so:
>
>ACK
Think of it as working around broken nbdkit behavior, rather than
fixing it (as the fix is in patch 1/4). How many stable branches
should we backport this to? Going all the way back to stable-1.2
seems like it might be overkill; maybe back to stable-1.14 (where
nbd-standalone was first split off) or stable-1.12 (the last release
that did not use libnbd) is sufficient?
I'm routinely backporting only to 1.16 and 1.18.
And then backporting to other branches only as-needed. For example I
backported something to 1.4 yesterday, but that was only because I was
trying to check for a bug which had been reported against 1.4 and I
needed the backport to make the branch compile on a modern distro.
By the way I'm happy to do these cherry picks (just for 1.16 and 1.18)
because I find it helps me to keep the commits in their original order
so I know which ones have been applied and which ones have been
skipped. Unless there is tooling which does this, which I've
occasionally looked for but never found.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
7:16 a.m.
New subject: [nbdkit PATCH 2/4] nbd: Normalize return values of can_*
On 3/17/20 7:10 AM, Richard W.M. Jones wrote:
On Tue, Mar 17, 2020 at 06:59:09AM -0500, Eric Blake wrote:
> On 3/17/20 3:12 AM, Richard W.M. Jones wrote:
>> On Mon, Mar 16, 2020 at 10:36:15PM -0500, Eric Blake wrote:
>>> Although nbdkit documents that any positive value should be treated as
>>> success to the .can_* callbacks, we had a window of releases where
>>> anything other than 1 could trigger an assertion failure, fixed in the
>>> previous patch. Update what we return to avoid tripping the bug in
>>> broken nbdkit.
>>>
>>> Our return value has been latent since 0e8e8eb11d (v1.1.17), but the
>>> assertion failures did not occur until c306fa93ab and neighboring
>>> commits (v1.15.1). As v1.13.6 was the point where we started
>>> preferring builds against libnbd, where we always returned 1 on
>>> success, the problem was not detected until now; but it is still in
>>> the wild for any user mixing old plugins with new libnbd.
>>>
>>> Signed-off-by: Eric Blake <eblake(a)redhat.com>
>>> ---
>>> plugins/nbd/nbd-standalone.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>> While I don't like the idea of modifying plugins to fix broken nbdkit
>> behaviour, you make a good case for this being safer, so:
>>
>> ACK
>
> Think of it as working around broken nbdkit behavior, rather than
> fixing it (as the fix is in patch 1/4). How many stable branches
> should we backport this to? Going all the way back to stable-1.2
> seems like it might be overkill; maybe back to stable-1.14 (where
> nbd-standalone was first split off) or stable-1.12 (the last release
> that did not use libnbd) is sufficient?
I'm routinely backporting only to 1.16 and 1.18.
And then backporting to other branches only as-needed. For example I
backported something to 1.4 yesterday, but that was only because I was
trying to check for a bug which had been reported against 1.4 and I
needed the backport to make the branch compile on a modern distro.
By the way I'm happy to do these cherry picks (just for 1.16 and 1.18)
because I find it helps me to keep the commits in their original order
so I know which ones have been applied and which ones have been
skipped. Unless there is tooling which does this, which I've
occasionally looked for but never found.
Okay, I'll let you handle the backports, including deciding how many (or
few) branches need which patches. Note that it is only 1.16 and 1.18
where we can trigger the assertion in-tree; backporting to any earlier
versions only helps for the case of cross-version mismatch which is less
common.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
10:36 p.m.
New subject: [nbdkit PATCH 3/4] tests: Don't let test-parallel-* hang on nbdkit bug
If nbdkit has a bug (such as the nbd-standalone bug fixed in the
previous commit), qemu-io ends up waiting forever rather than
realizing that if the server disappears unexpectedly then qemu-io
should quit. So add timeouts so the testsuite will flag the problem
instead of hang (tested by reordering this commit before the
previous).
It's trickier than I expected: from the command line, 'timeout 10s
qemu-io ...' works just fine, but from a script, it fails. Why?
Because timeout defaults to creating a new process group when run
non-interactively, but qemu-io is generally an interactive program
that expects to be able to modify the terminal settings when stdin is
a terminal, even when it will not be reading from stdin because of the
use of -c. When stdin is inherited but no longer the controlling
terminal because of timeout's new process group, qemu-io fails to make
progress. As solutions, we can either use 'timeout --foreground', or
redirect stdin to something that is not a terminal; this patch uses
the latter.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
tests/test-parallel-file.sh | 19 ++++++++++---------
tests/test-parallel-nbd.sh | 13 ++++++++-----
tests/test-parallel-sh.sh | 19 ++++++++++---------
3 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/tests/test-parallel-file.sh b/tests/test-parallel-file.sh
index 20276d48..136c2db5 100755
--- a/tests/test-parallel-file.sh
+++ b/tests/test-parallel-file.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2017-2019 Red Hat Inc.
+# Copyright (C) 2017-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@ source ./functions.sh
# Check file-data was created by Makefile and qemu-io exists.
requires test -f file-data
requires qemu-io --version
+requires timeout --version
nbdkit --dump-plugin file | grep -q ^thread_model=parallel ||
{ echo "nbdkit lacks support for parallel requests"; exit 77; }
@@ -43,8 +44,8 @@ cleanup_fn rm -f test-parallel-file.data test-parallel-file.out
# Populate file, and sanity check that qemu-io can issue parallel requests
printf '%1024s' . > test-parallel-file.data
-qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512 512"
\
- -c aio_flush test-parallel-file.data ||
+timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \
+ -c "aio_write -P 2 512 512" -c aio_flush test-parallel-file.data ||
{ echo "'qemu-io' can't drive parallel requests"; exit 77; }
# Set up the file plugin to delay both reads and writes (for a good chance
@@ -55,8 +56,8 @@ qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P
2 512 512" \
# With --threads=1, the write should complete first because it was issued first
nbdkit -v -t 1 -U - --filter=delay file test-parallel-file.data \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
tee test-parallel-file.out
if test "$(grep '512/512' test-parallel-file.out)" != \
"wrote 512/512 bytes at offset 512
@@ -66,8 +67,8 @@ fi
# With default --threads, the faster read should complete first
nbdkit -v -U - --filter=delay file test-parallel-file.data \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
tee test-parallel-file.out
if test "$(grep '512/512' test-parallel-file.out)" != \
"read 512/512 bytes at offset 0
@@ -77,8 +78,8 @@ fi
# With --filter=noparallel, the write should complete first because it was issued first
nbdkit -v -U - --filter=noparallel --filter=delay file test-parallel-file.data \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
tee test-parallel-file.out
if test "$(grep '512/512' test-parallel-file.out)" != \
"wrote 512/512 bytes at offset 512
diff --git a/tests/test-parallel-nbd.sh b/tests/test-parallel-nbd.sh
index 4e546df4..1fdf7df4 100755
--- a/tests/test-parallel-nbd.sh
+++ b/tests/test-parallel-nbd.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2017-2019 Red Hat Inc.
+# Copyright (C) 2017-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@ source ./functions.sh
# Check file-data was created by Makefile and qemu-io exists.
requires test -f file-data
requires qemu-io --version
+requires timeout --version
nbdkit --dump-plugin nbd | grep -q ^thread_model=parallel ||
{ echo "nbdkit lacks support for parallel requests"; exit 77; }
@@ -46,8 +47,8 @@ cleanup_fn rm -f $files
# Populate file, and sanity check that qemu-io can issue parallel requests
printf '%1024s' . > test-parallel-nbd.data
-qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512 512"
\
- -c aio_flush test-parallel-nbd.data ||
+timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \
+ -c "aio_write -P 2 512 512" -c aio_flush test-parallel-nbd.data ||
{ echo "'qemu-io' can't drive parallel requests"; exit 77; }
# Set up the file plugin to delay both reads and writes (for a good chance
@@ -63,7 +64,8 @@ start_nbdkit -P test-parallel-nbd.pid \
# With --threads=1, the write should complete first because it was issued first
nbdkit -v -t 1 -U - nbd socket=$sock --run '
- qemu-io -f raw -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512"
\
+ timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" \
-c aio_flush $nbd' | tee test-parallel-nbd.out
if test "$(grep '512/512' test-parallel-nbd.out)" != \
"wrote 512/512 bytes at offset 512
@@ -73,7 +75,8 @@ fi
# With default --threads, the faster read should complete first
nbdkit -v -U - nbd socket=$sock --run '
- qemu-io -f raw -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512"
\
+ timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" \
-c aio_flush $nbd' | tee test-parallel-nbd.out
if test "$(grep '512/512' test-parallel-nbd.out)" != \
"read 512/512 bytes at offset 0
diff --git a/tests/test-parallel-sh.sh b/tests/test-parallel-sh.sh
index 15d8875d..4cc93c33 100755
--- a/tests/test-parallel-sh.sh
+++ b/tests/test-parallel-sh.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2017-2019 Red Hat Inc.
+# Copyright (C) 2017-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@ source ./functions.sh
# Check file-data was created by Makefile and qemu-io exists.
requires test -f file-data
requires qemu-io --version
+requires timeout --version
nbdkit --dump-plugin sh | grep -q ^thread_model=parallel ||
{ echo "nbdkit lacks support for parallel requests"; exit 77; }
@@ -43,8 +44,8 @@ cleanup_fn rm -f test-parallel-sh.data test-parallel-sh.out
test-parallel-sh.scr
# Populate file, and sanity check that qemu-io can issue parallel requests
printf '%1024s' . > test-parallel-sh.data
-qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512 512"
\
- -c aio_flush test-parallel-sh.data ||
+timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \
+ -c "aio_write -P 2 512 512" -c aio_flush test-parallel-sh.data ||
{ echo "'qemu-io' can't drive parallel requests"; exit 77; }
# Set up the sh plugin to delay both reads and writes (for a good
@@ -96,8 +97,8 @@ chmod +x test-parallel-sh.script
# With --threads=1, the write should complete first because it was issued first
nbdkit -v -t 1 -U - --filter=delay sh test-parallel-sh.script \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
tee test-parallel-sh.out
if test "$(grep '512/512' test-parallel-sh.out)" != \
"wrote 512/512 bytes at offset 512
@@ -107,8 +108,8 @@ fi
# With default --threads, the faster read should complete first
nbdkit -v -U - --filter=delay sh test-parallel-sh.script \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
tee test-parallel-sh.out
if test "$(grep '512/512' test-parallel-sh.out)" != \
"read 512/512 bytes at offset 0
@@ -120,8 +121,8 @@ fi
# issued first. Also test that the log filter doesn't leak an fd
nbdkit -v -U - --filter=noparallel --filter=log --filter=delay \
sh test-parallel-sh.script logfile=/dev/null \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
tee test-parallel-sh.out
if test "$(grep '512/512' test-parallel-sh.out)" != \
"wrote 512/512 bytes at offset 512
--
2.25.1
3:16 a.m.
New subject: [nbdkit PATCH 3/4] tests: Don't let test-parallel-* hang on nbdkit bug
On Mon, Mar 16, 2020 at 10:36:16PM -0500, Eric Blake wrote:
If nbdkit has a bug (such as the nbd-standalone bug fixed in the
previous commit), qemu-io ends up waiting forever rather than
realizing that if the server disappears unexpectedly then qemu-io
should quit. So add timeouts so the testsuite will flag the problem
instead of hang (tested by reordering this commit before the
previous).
It's trickier than I expected: from the command line, 'timeout 10s
qemu-io ...' works just fine, but from a script, it fails. Why?
Because timeout defaults to creating a new process group when run
non-interactively, but qemu-io is generally an interactive program
that expects to be able to modify the terminal settings when stdin is
a terminal, even when it will not be reading from stdin because of the
use of -c. When stdin is inherited but no longer the controlling
terminal because of timeout's new process group, qemu-io fails to make
progress. As solutions, we can either use 'timeout --foreground', or
redirect stdin to something that is not a terminal; this patch uses
the latter.
Since the timeout is only used here as a last resort, can
we make it much larger? This is only going to cause weird
test failures on the usual suspects (z, armv7, valgrind, ...)
The timeout could be 60s or even 5 minutes without it affecting
normal users.
With a change like that, ACK.
Rich.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
tests/test-parallel-file.sh | 19 ++++++++++---------
tests/test-parallel-nbd.sh | 13 ++++++++-----
tests/test-parallel-sh.sh | 19 ++++++++++---------
3 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/tests/test-parallel-file.sh b/tests/test-parallel-file.sh
index 20276d48..136c2db5 100755
--- a/tests/test-parallel-file.sh
+++ b/tests/test-parallel-file.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2017-2019 Red Hat Inc.
+# Copyright (C) 2017-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@ source ./functions.sh
# Check file-data was created by Makefile and qemu-io exists.
requires test -f file-data
requires qemu-io --version
+requires timeout --version
nbdkit --dump-plugin file | grep -q ^thread_model=parallel ||
{ echo "nbdkit lacks support for parallel requests"; exit 77; }
@@ -43,8 +44,8 @@ cleanup_fn rm -f test-parallel-file.data test-parallel-file.out
# Populate file, and sanity check that qemu-io can issue parallel requests
printf '%1024s' . > test-parallel-file.data
-qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512 512"
\
- -c aio_flush test-parallel-file.data ||
+timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \
+ -c "aio_write -P 2 512 512" -c aio_flush test-parallel-file.data ||
{ echo "'qemu-io' can't drive parallel requests"; exit 77; }
# Set up the file plugin to delay both reads and writes (for a good chance
@@ -55,8 +56,8 @@ qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write
-P 2 512 512" \
# With --threads=1, the write should complete first because it was issued first
nbdkit -v -t 1 -U - --filter=delay file test-parallel-file.data \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-file.out
if test "$(grep '512/512' test-parallel-file.out)" != \
"wrote 512/512 bytes at offset 512
@@ -66,8 +67,8 @@ fi
# With default --threads, the faster read should complete first
nbdkit -v -U - --filter=delay file test-parallel-file.data \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-file.out
if test "$(grep '512/512' test-parallel-file.out)" != \
"read 512/512 bytes at offset 0
@@ -77,8 +78,8 @@ fi
# With --filter=noparallel, the write should complete first because it was issued first
nbdkit -v -U - --filter=noparallel --filter=delay file test-parallel-file.data \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-file.out
if test "$(grep '512/512' test-parallel-file.out)" != \
"wrote 512/512 bytes at offset 512
diff --git a/tests/test-parallel-nbd.sh b/tests/test-parallel-nbd.sh
index 4e546df4..1fdf7df4 100755
--- a/tests/test-parallel-nbd.sh
+++ b/tests/test-parallel-nbd.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2017-2019 Red Hat Inc.
+# Copyright (C) 2017-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@ source ./functions.sh
# Check file-data was created by Makefile and qemu-io exists.
requires test -f file-data
requires qemu-io --version
+requires timeout --version
nbdkit --dump-plugin nbd | grep -q ^thread_model=parallel ||
{ echo "nbdkit lacks support for parallel requests"; exit 77; }
@@ -46,8 +47,8 @@ cleanup_fn rm -f $files
# Populate file, and sanity check that qemu-io can issue parallel requests
printf '%1024s' . > test-parallel-nbd.data
-qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512 512"
\
- -c aio_flush test-parallel-nbd.data ||
+timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \
+ -c "aio_write -P 2 512 512" -c aio_flush test-parallel-nbd.data ||
{ echo "'qemu-io' can't drive parallel requests"; exit 77; }
# Set up the file plugin to delay both reads and writes (for a good chance
@@ -63,7 +64,8 @@ start_nbdkit -P test-parallel-nbd.pid \
# With --threads=1, the write should complete first because it was issued first
nbdkit -v -t 1 -U - nbd socket=$sock --run '
- qemu-io -f raw -c "aio_write -P 2 512 512" -c "aio_read -P 1 0
512" \
+ timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" \
-c aio_flush $nbd' | tee test-parallel-nbd.out
if test "$(grep '512/512' test-parallel-nbd.out)" != \
"wrote 512/512 bytes at offset 512
@@ -73,7 +75,8 @@ fi
# With default --threads, the faster read should complete first
nbdkit -v -U - nbd socket=$sock --run '
- qemu-io -f raw -c "aio_write -P 2 512 512" -c "aio_read -P 1 0
512" \
+ timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" \
-c aio_flush $nbd' | tee test-parallel-nbd.out
if test "$(grep '512/512' test-parallel-nbd.out)" != \
"read 512/512 bytes at offset 0
diff --git a/tests/test-parallel-sh.sh b/tests/test-parallel-sh.sh
index 15d8875d..4cc93c33 100755
--- a/tests/test-parallel-sh.sh
+++ b/tests/test-parallel-sh.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2017-2019 Red Hat Inc.
+# Copyright (C) 2017-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@ source ./functions.sh
# Check file-data was created by Makefile and qemu-io exists.
requires test -f file-data
requires qemu-io --version
+requires timeout --version
nbdkit --dump-plugin sh | grep -q ^thread_model=parallel ||
{ echo "nbdkit lacks support for parallel requests"; exit 77; }
@@ -43,8 +44,8 @@ cleanup_fn rm -f test-parallel-sh.data test-parallel-sh.out
test-parallel-sh.scr
# Populate file, and sanity check that qemu-io can issue parallel requests
printf '%1024s' . > test-parallel-sh.data
-qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512 512"
\
- -c aio_flush test-parallel-sh.data ||
+timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \
+ -c "aio_write -P 2 512 512" -c aio_flush test-parallel-sh.data ||
{ echo "'qemu-io' can't drive parallel requests"; exit 77; }
# Set up the sh plugin to delay both reads and writes (for a good
@@ -96,8 +97,8 @@ chmod +x test-parallel-sh.script
# With --threads=1, the write should complete first because it was issued first
nbdkit -v -t 1 -U - --filter=delay sh test-parallel-sh.script \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-sh.out
if test "$(grep '512/512' test-parallel-sh.out)" != \
"wrote 512/512 bytes at offset 512
@@ -107,8 +108,8 @@ fi
# With default --threads, the faster read should complete first
nbdkit -v -U - --filter=delay sh test-parallel-sh.script \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-sh.out
if test "$(grep '512/512' test-parallel-sh.out)" != \
"read 512/512 bytes at offset 0
@@ -120,8 +121,8 @@ fi
# issued first. Also test that the log filter doesn't leak an fd
nbdkit -v -U - --filter=noparallel --filter=log --filter=delay \
sh test-parallel-sh.script logfile=/dev/null \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \
- -c "aio_read -P 1 0 512" -c aio_flush $nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-sh.out
if test "$(grep '512/512' test-parallel-sh.out)" != \
"wrote 512/512 bytes at offset 512
--
2.25.1
_______________________________________________
Libguestfs mailing list
Libguestfs(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
--
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
10:36 p.m.
New subject: [nbdkit PATCH 4/4] RFC tests: Add test to cover unusual .can_flush return
We want some testsuite coverage of handling non-1 .can_flush values.
The only in-tree plugin with this property is nbd-standalone, but it
doesn't get frequently tested, not to mention that the next patch will
change it to work with older nbdkit.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
work in progress: currently compiles but fails during test; posting it
now to show my thoughts before I go to bed
tests/Makefile.am | 21 +++++++
tests/test-flush.sh | 85 +++++++++++++++++++++++++++
tests/test-flush-plugin.c | 119 ++++++++++++++++++++++++++++++++++++++
3 files changed, 225 insertions(+)
create mode 100755 tests/test-flush.sh
create mode 100644 tests/test-flush-plugin.c
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 65dd148d..7e0024d3 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -116,6 +116,7 @@ EXTRA_DIST = \
test-extentlist.sh \
test-file-extents.sh \
test-floppy.sh \
+ test-flush.sh \
test-foreground.sh \
test-fua.sh \
test-full.sh \
@@ -267,6 +268,7 @@ TESTS += \
test-foreground.sh \
test-debug-flags.sh \
test-long-name.sh \
+ test-flush.sh \
test-swap.sh \
test-shutdown.sh \
$(NULL)
@@ -282,6 +284,25 @@ test_socket_activation_CPPFLAGS = \
$(NULL)
test_socket_activation_CFLAGS = $(WARNINGS_CFLAGS)
+# check_LTLIBRARIES won't build a shared library (see automake manual).
+# So we have to do this and add a dependency.
+noinst_LTLIBRARIES += \
+ test-flush-plugin.la \
+ $(NULL)
+test-flush.sh: test-flush-plugin.la
+
+test_flush_plugin_la_SOURCES = \
+ test-flush-plugin.c \
+ $(top_srcdir)/include/nbdkit-plugin.h \
+ $(NULL)
+test_flush_plugin_la_CPPFLAGS = -I$(top_srcdir)/include
+test_flush_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
+# For use of the -rpath option, see:
+# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html
+test_flush_plugin_la_LDFLAGS = \
+ -module -avoid-version -shared -rpath /nowhere \
+ $(NULL)
+
# check_LTLIBRARIES won't build a shared library (see automake manual).
# So we have to do this and add a dependency.
noinst_LTLIBRARIES += \
diff --git a/tests/test-flush.sh b/tests/test-flush.sh
new file mode 100755
index 00000000..54801b3a
--- /dev/null
+++ b/tests/test-flush.sh
@@ -0,0 +1,85 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -x
+
+requires nbdsh --version
+
+plugin=.libs/test-flush-plugin.so
+requires test -f $plugin
+
+# Test what happens when a plugin fails .can_flush
+nbdsh -c '
+try:
+ h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "level=-1"])
+except nbd.Error as ex:
+ exit (0)
+# If we got here, things are broken
+exit (1)
+'
+
+# A read-only connection never triggers .can_flush
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-r", "-v",
"'$plugin'", "level=-1"])
+assert h.is_read_only () == 1
+assert h.can_flush () == 0
+assert h.can_fua () == 0
+'
+
+# Disable flush and FUA
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "0"])
+assert h.is_read_only () == 0
+assert h.can_flush () == 0
+assert h.can_fua () == 0
+'
+
+# Normal flush, emulated FUA
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "1"])
+assert h.is_read_only () == 0
+assert h.can_flush () == 1
+assert h.can_fua () == 1
+# try direct flush; check that output has "handling flush"
+# try write with FUA; check that output has "handling flush"
+'
+
+# Unusual return value for .can_flush, native FUA
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "2"])
+assert h.is_read_only () == 0
+assert h.can_flush () == 1
+assert h.can_fua () == 1
+# try direct flush; check that output has "handling flush"
+# try write with FUA; check that output has "handling native FUA"
+'
diff --git a/tests/test-flush-plugin.c b/tests/test-flush-plugin.c
new file mode 100644
index 00000000..5553c607
--- /dev/null
+++ b/tests/test-flush-plugin.c
@@ -0,0 +1,119 @@
+/* nbdkit
+ * Copyright (C) 2013-2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#define NBDKIT_API_VERSION 2
+
+#include <nbdkit-plugin.h>
+
+/* level abuses our knowledge of internal nbdkit values:
+ * -1: force error during connect
+ * 0: no flush, no FUA
+ * 1: flush works, FUA is emulated
+ * 2: flush works, FUA is native
+ */
+static int level;
+
+static int
+flush_config (const char *key, const char *value)
+{
+ if (strcmp (key, "level") == 0)
+ return nbdkit_parse_int (key, value, &level);
+ nbdkit_error ("unknown parameter '%s'", key);
+ return -1;
+}
+
+/* Implements both .can_flush and .can_fua */
+static int
+flush_level (void *handle)
+{
+ return level;
+}
+
+static void *
+flush_open (int readonly)
+{
+ return NBDKIT_HANDLE_NOT_NEEDED;
+}
+
+static int64_t
+flush_get_size (void *handle)
+{
+ return 1024*1024;
+}
+
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
+
+static int
+flush_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags)
+{
+ memset (buf, 0, count);
+ return 0;
+}
+
+static int
+flush_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags)
+{
+ if (flags & NBDKIT_FLAG_FUA)
+ nbdkit_debug ("handling native FUA");
+ return 0;
+}
+
+static int
+flush_flush (void *handle, uint32_t flags)
+{
+ nbdkit_debug ("handling flush");
+ return 0;
+}
+
+static struct nbdkit_plugin plugin = {
+ .name = "flush",
+ .version = PACKAGE_VERSION,
+ .config = flush_config,
+ .magic_config_key = "level",
+ .open = flush_open,
+ .get_size = flush_get_size,
+ .pread = flush_pread,
+ .pwrite = flush_pwrite,
+ .can_flush = flush_level,
+ .can_fua = flush_level,
+ .flush = flush_flush,
+};
+
+NBDKIT_REGISTER_PLUGIN(plugin)
--
2.25.1
3:19 a.m.
New subject: [nbdkit PATCH 4/4] RFC tests: Add test to cover unusual .can_flush return
On Mon, Mar 16, 2020 at 10:36:17PM -0500, Eric Blake wrote:
We want some testsuite coverage of handling non-1 .can_flush values.
The only in-tree plugin with this property is nbd-standalone, but it
doesn't get frequently tested, not to mention that the next patch will
change it to work with older nbdkit.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
work in progress: currently compiles but fails during test; posting it
now to show my thoughts before I go to bed
Yes, it all looks reasonable so far.
Rich.
tests/Makefile.am | 21 +++++++
tests/test-flush.sh | 85 +++++++++++++++++++++++++++
tests/test-flush-plugin.c | 119 ++++++++++++++++++++++++++++++++++++++
3 files changed, 225 insertions(+)
create mode 100755 tests/test-flush.sh
create mode 100644 tests/test-flush-plugin.c
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 65dd148d..7e0024d3 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -116,6 +116,7 @@ EXTRA_DIST = \
test-extentlist.sh \
test-file-extents.sh \
test-floppy.sh \
+ test-flush.sh \
test-foreground.sh \
test-fua.sh \
test-full.sh \
@@ -267,6 +268,7 @@ TESTS += \
test-foreground.sh \
test-debug-flags.sh \
test-long-name.sh \
+ test-flush.sh \
test-swap.sh \
test-shutdown.sh \
$(NULL)
@@ -282,6 +284,25 @@ test_socket_activation_CPPFLAGS = \
$(NULL)
test_socket_activation_CFLAGS = $(WARNINGS_CFLAGS)
+# check_LTLIBRARIES won't build a shared library (see automake manual).
+# So we have to do this and add a dependency.
+noinst_LTLIBRARIES += \
+ test-flush-plugin.la \
+ $(NULL)
+test-flush.sh: test-flush-plugin.la
+
+test_flush_plugin_la_SOURCES = \
+ test-flush-plugin.c \
+ $(top_srcdir)/include/nbdkit-plugin.h \
+ $(NULL)
+test_flush_plugin_la_CPPFLAGS = -I$(top_srcdir)/include
+test_flush_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
+# For use of the -rpath option, see:
+# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html
+test_flush_plugin_la_LDFLAGS = \
+ -module -avoid-version -shared -rpath /nowhere \
+ $(NULL)
+
# check_LTLIBRARIES won't build a shared library (see automake manual).
# So we have to do this and add a dependency.
noinst_LTLIBRARIES += \
diff --git a/tests/test-flush.sh b/tests/test-flush.sh
new file mode 100755
index 00000000..54801b3a
--- /dev/null
+++ b/tests/test-flush.sh
@@ -0,0 +1,85 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -x
+
+requires nbdsh --version
+
+plugin=.libs/test-flush-plugin.so
+requires test -f $plugin
+
+# Test what happens when a plugin fails .can_flush
+nbdsh -c '
+try:
+ h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "level=-1"])
+except nbd.Error as ex:
+ exit (0)
+# If we got here, things are broken
+exit (1)
+'
+
+# A read-only connection never triggers .can_flush
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-r", "-v",
"'$plugin'", "level=-1"])
+assert h.is_read_only () == 1
+assert h.can_flush () == 0
+assert h.can_fua () == 0
+'
+
+# Disable flush and FUA
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "0"])
+assert h.is_read_only () == 0
+assert h.can_flush () == 0
+assert h.can_fua () == 0
+'
+
+# Normal flush, emulated FUA
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "1"])
+assert h.is_read_only () == 0
+assert h.can_flush () == 1
+assert h.can_fua () == 1
+# try direct flush; check that output has "handling flush"
+# try write with FUA; check that output has "handling flush"
+'
+
+# Unusual return value for .can_flush, native FUA
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "2"])
+assert h.is_read_only () == 0
+assert h.can_flush () == 1
+assert h.can_fua () == 1
+# try direct flush; check that output has "handling flush"
+# try write with FUA; check that output has "handling native FUA"
+'
diff --git a/tests/test-flush-plugin.c b/tests/test-flush-plugin.c
new file mode 100644
index 00000000..5553c607
--- /dev/null
+++ b/tests/test-flush-plugin.c
@@ -0,0 +1,119 @@
+/* nbdkit
+ * Copyright (C) 2013-2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#define NBDKIT_API_VERSION 2
+
+#include <nbdkit-plugin.h>
+
+/* level abuses our knowledge of internal nbdkit values:
+ * -1: force error during connect
+ * 0: no flush, no FUA
+ * 1: flush works, FUA is emulated
+ * 2: flush works, FUA is native
+ */
+static int level;
+
+static int
+flush_config (const char *key, const char *value)
+{
+ if (strcmp (key, "level") == 0)
+ return nbdkit_parse_int (key, value, &level);
+ nbdkit_error ("unknown parameter '%s'", key);
+ return -1;
+}
+
+/* Implements both .can_flush and .can_fua */
+static int
+flush_level (void *handle)
+{
+ return level;
+}
+
+static void *
+flush_open (int readonly)
+{
+ return NBDKIT_HANDLE_NOT_NEEDED;
+}
+
+static int64_t
+flush_get_size (void *handle)
+{
+ return 1024*1024;
+}
+
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
+
+static int
+flush_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags)
+{
+ memset (buf, 0, count);
+ return 0;
+}
+
+static int
+flush_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags)
+{
+ if (flags & NBDKIT_FLAG_FUA)
+ nbdkit_debug ("handling native FUA");
+ return 0;
+}
+
+static int
+flush_flush (void *handle, uint32_t flags)
+{
+ nbdkit_debug ("handling flush");
+ return 0;
+}
+
+static struct nbdkit_plugin plugin = {
+ .name = "flush",
+ .version = PACKAGE_VERSION,
+ .config = flush_config,
+ .magic_config_key = "level",
+ .open = flush_open,
+ .get_size = flush_get_size,
+ .pread = flush_pread,
+ .pwrite = flush_pwrite,
+ .can_flush = flush_level,
+ .can_fua = flush_level,
+ .flush = flush_flush,
+};
+
+NBDKIT_REGISTER_PLUGIN(plugin)
--
2.25.1
_______________________________________________
Libguestfs mailing list
Libguestfs(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
--
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
3:09 a.m.
On Mon, Mar 16, 2020 at 10:36:13PM -0500, Eric Blake wrote:
Either patch 1 or patch 2 in isolation is sufficient to fix the
problem that Rich forwarded on from an archlinux tester (name so I can
credit them?).
Svenne Krap (https://svenne.dk)
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
1710
days inactive
1710
days old
13 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones