Re: [Libguestfs] [nbdkit PATCH 1/2] retry: Add in retry support during .open
On Fri, Jan 27, 2023 at 02:41:22PM -0600, Eric Blake wrote:
> Now that a filter can open a backend as many times as it wants,
> there's no longer a technical reason we can't retry .open. However,
> adding retry logic here does mean we have to weaken an assert in the
> server backend code, since prepare can now be reached more than once.
>
> Test coverage will be added in a separate patch, so that it becomes
> easy to swap patch order and see that the test fails without this
> patch.
>
> See also https://bugzilla.redhat.com/show_bug.cgi?id=1841820
> ---
> +static void *
> +retry_open (nbdkit_next_open *next, nbdkit_context *nxdata,
> +int readonly, const char *exportname, int is_tls)
> +{
> + struct retry_handle *h;
> + struct retry_data data = {0};
> +
> + h = malloc (sizeof *h);
Uninit memory...
> + if (h == NULL) {
> +nbdkit_error ("malloc: %m");
> +return NULL;
> + }
> +
> + h->readonly = readonly;
> + h->exportname = strdup (exportname);
> + h->context = nxdata;
> + if (h->exportname == NULL) {
> +nbdkit_error ("strdup: %m");
> +free (h);
> +return NULL;
> + }
> + h->reopens = 0;
> +
> + if (next (nxdata, readonly, exportname) != -1)
> +h->open = true;
> + else {
and h->open has not been assigned if we get here...
> +/* Careful - our .open must not return a handle unless do_retry()
> + * works, as the caller's next action will be calling .get_size
> + * and similar probe functions which we do not bother to wire up
> + * into retry logic because they only need to be used right after
> + * connecting.
> + */
> +nbdkit_next *next_handle = NULL;
> +int err = ESHUTDOWN;
> +
> +while (! h->open && do_retry (h, &data, &next_handle, "open", &err))
...even though I assumed it was zero-initialized here. Will fix.
--
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
[Libguestfs] [nbdkit PATCH 2/2] retry: Test previous patch
Separate commit to allow rearranging patches to demonstrate that open can now be retried. --- tests/Makefile.am| 2 + tests/test-retry-open.sh | 85 2 files changed, 87 insertions(+) create mode 100755 tests/test-retry-open.sh diff --git a/tests/Makefile.am b/tests/Makefile.am index e5396b5f..713ccca7 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1807,6 +1807,7 @@ TESTS += \ test-retry-size.sh \ test-retry-reopen-fail.sh \ test-retry-zero-flags.sh \ + test-retry-open.sh \ $(NULL) EXTRA_DIST += \ test-retry.sh \ @@ -1815,6 +1816,7 @@ EXTRA_DIST += \ test-retry-size.sh \ test-retry-reopen-fail.sh \ test-retry-zero-flags.sh \ + test-retry-open.sh \ $(NULL) # retry-request filter test. diff --git a/tests/test-retry-open.sh b/tests/test-retry-open.sh new file mode 100755 index ..468a03e1 --- /dev/null +++ b/tests/test-retry-open.sh @@ -0,0 +1,85 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2018-2023 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 -e +set -x + +requires_plugin sh +requires qemu-io --version + +files="retry-open-count" +rm -f $files +cleanup_fn rm -f $files + +echo 0 > retry-open-count +start_t=$SECONDS + +# Create a custom plugin which will test retrying open. +nbdkit -v -U - \ + sh - \ + --filter=retry retry-delay=1 \ + --run 'qemu-io -f raw -c "r -P0 0 512" $nbd || :' <<'EOF' +#!/usr/bin/env bash +case "$1" in +open) +# Count how many times the connection is (re-)opened. +read i < retry-open-count +echo $((i+1)) > retry-open-count +if test $i -lt 1; then + echo EIO >&2; exit 1 +fi +;; +get_size) echo 512 ;; +pread) dd if=/dev/zero count=$3 iflag=count_bytes ;; +*) exit 2 ;; +esac +EOF + +# In this test we should see 1 failure: +# first open FAILS +# retry and wait 1 second +# second open PASSES + +# The minimum time for the test should be 1 second. +end_t=$SECONDS +if [ $((end_t - start_t)) -lt 1 ]; then +echo "$0: test ran too quickly" +exit 1 +fi + +# Check the handle was opened 2 times (first open + one reopen). +read open_count < retry-open-count +if [ $open_count -ne 2 ]; then +echo "$0: open-count ($open_count) != 2" +exit 1 +fi -- 2.39.1 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [nbdkit PATCH 1/2] retry: Add in retry support during .open
Now that a filter can open a backend as many times as it wants,
there's no longer a technical reason we can't retry .open. However,
adding retry logic here does mean we have to weaken an assert in the
server backend code, since prepare can now be reached more than once.
Test coverage will be added in a separate patch, so that it becomes
easy to swap patch order and see that the test fails without this
patch.
See also https://bugzilla.redhat.com/show_bug.cgi?id=1841820
---
server/backend.c | 7 +++-
filters/retry/retry.c | 98 +--
2 files changed, 63 insertions(+), 42 deletions(-)
diff --git a/server/backend.c b/server/backend.c
index 3bbba601..45889ea9 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2021 Red Hat Inc.
+ * Copyright (C) 2013-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -312,7 +312,10 @@ backend_prepare (struct context *c)
struct backend *b = c->b;
assert (c->handle);
- assert ((c->state & (HANDLE_OPEN | HANDLE_CONNECTED)) == HANDLE_OPEN);
+ assert (c->state & HANDLE_OPEN);
+
+ if (c->state & HANDLE_CONNECTED)
+return 0;
/* Call these in order starting from the filter closest to the
* plugin, similar to typical .open order. But remember that
diff --git a/filters/retry/retry.c b/filters/retry/retry.c
index fb1d0526..7abf74c4 100644
--- a/filters/retry/retry.c
+++ b/filters/retry/retry.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2019-2021 Red Hat Inc.
+ * Copyright (C) 2019-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -113,45 +113,6 @@ struct retry_handle {
bool open;
};
-static void *
-retry_open (nbdkit_next_open *next, nbdkit_context *nxdata,
-int readonly, const char *exportname, int is_tls)
-{
- struct retry_handle *h;
-
- if (next (nxdata, readonly, exportname) == -1)
-return NULL;
-
- h = malloc (sizeof *h);
- if (h == NULL) {
-nbdkit_error ("malloc: %m");
-return NULL;
- }
-
- h->readonly = readonly;
- h->exportname = strdup (exportname);
- h->context = nxdata;
- if (h->exportname == NULL) {
-nbdkit_error ("strdup: %m");
-free (h);
-return NULL;
- }
- h->reopens = 0;
- h->open = true;
-
- return h;
-}
-
-static void
-retry_close (void *handle)
-{
- struct retry_handle *h = handle;
-
- nbdkit_debug ("reopens needed: %u", h->reopens);
- free (h->exportname);
- free (h);
-}
-
/* This function encapsulates the common retry logic used across all
* data commands. If it returns true then the data command will retry
* the operation. âstruct retry_dataâ is stack data saved between
@@ -247,6 +208,63 @@ do_retry (struct retry_handle *h, struct retry_data *data,
return true;
}
+static void *
+retry_open (nbdkit_next_open *next, nbdkit_context *nxdata,
+int readonly, const char *exportname, int is_tls)
+{
+ struct retry_handle *h;
+ struct retry_data data = {0};
+
+ h = malloc (sizeof *h);
+ if (h == NULL) {
+nbdkit_error ("malloc: %m");
+return NULL;
+ }
+
+ h->readonly = readonly;
+ h->exportname = strdup (exportname);
+ h->context = nxdata;
+ if (h->exportname == NULL) {
+nbdkit_error ("strdup: %m");
+free (h);
+return NULL;
+ }
+ h->reopens = 0;
+
+ if (next (nxdata, readonly, exportname) != -1)
+h->open = true;
+ else {
+/* Careful - our .open must not return a handle unless do_retry()
+ * works, as the caller's next action will be calling .get_size
+ * and similar probe functions which we do not bother to wire up
+ * into retry logic because they only need to be used right after
+ * connecting.
+ */
+nbdkit_next *next_handle = NULL;
+int err = ESHUTDOWN;
+
+while (! h->open && do_retry (h, &data, &next_handle, "open", &err))
+ ;
+
+if (! h->open) {
+ free (h->exportname);
+ free (h);
+ return NULL;
+}
+ }
+ return h;
+}
+
+static void
+retry_close (void *handle)
+{
+ struct retry_handle *h = handle;
+
+ nbdkit_debug ("reopens needed: %u", h->reopens);
+ free (h->exportname);
+ free (h);
+}
+
static int
retry_pread (nbdkit_next *next,
void *handle, void *buf, uint32_t count, uint64_t offset,
--
2.39.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [nbdkit PATCH 0/2] retry: add support for retrying .open
In https://bugzilla.redhat.com/show_bug.cgi?id=1841820, it was pointed out that the retry filter not retrying .open means that an ssh connection (such as in a vmx+ssh v2v conversion) fails when the ssh connection itself cannot be retried. A year ago, this was an inherent limitation of our retry implementation; but in the meantime, my work to allow filters to open independent backends has made it feasible to implement. Eric Blake (2): retry: Add in retry support during .open retry: Test previous patch tests/Makefile.am| 2 + server/backend.c | 7 ++- filters/retry/retry.c| 98 tests/test-retry-open.sh | 85 ++ 4 files changed, 150 insertions(+), 42 deletions(-) create mode 100755 tests/test-retry-open.sh -- 2.39.1 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Proposal for tools to inspect drivers and inject Windows virtio drivers
On Thu, Jan 19, 2023 at 06:49:32PM +, Richard W.M. Jones wrote: > On Sat, Dec 10, 2022 at 09:45:11PM +, Richard W.M. Jones wrote: > > (a) Inspecting a virtual machine disk image to find out what virtual > > devices it needs (ie. what drivers are installed in the guest). > > guestfs-tools 1.49.9 contains a new tool called 'virt-drivers' which > has this functionality extracted from virt-v2v. It's Linux only at > the moment (like virt-v2v). Implementing this for Windows is TBD. The virt-drivers tool supports listing Windows drivers starting with 1.49.10, so for example you can use this to tell if a Windows guest has virtio drivers (or VMware drivers) suitable for the target hypervisor. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH v2v v2] -o rhv-upload: Improve error message for invalid or missing -os parameter
For -o rhv-upload, the -os parameter specifies the storage domain.
Because the RHV API allows globs when searching for a domain, if you
used a parameter like -os 'data*' then this would confuse the Python
code, since it can glob to the name of a storage domain, but then
later fail when we try to exact match the storage domain we found.
The result of this was a confusing error in the precheck script:
IndexError: list index out of range
This fix validates the output storage parameter before trying to use
it. Since valid storage domain names cannot contain glob characters
or spaces, it avoids the problems above and improves the error message
that the user sees:
$ virt-v2v [...] -o rhv-upload -os ''
...
RuntimeError: The storage domain (-os) parameter ‘’ is not valid
virt-v2v: error: failed server prechecks, see earlier errors
$ virt-v2v [...] -o rhv-upload -os 'data*'
...
RuntimeError: The storage domain (-os) parameter ‘data*’ is not valid
virt-v2v: error: failed server prechecks, see earlier errors
Although the IndexError should no longer happen, I also added a
try...catch around that code to improve the error in case it still
happens.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1986386
Reported-by: Junqin Zhou
Thanks: Nir Soffer
---
output/rhv-upload-precheck.py | 27 +--
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/output/rhv-upload-precheck.py b/output/rhv-upload-precheck.py
index 1dc1b8498a..ba125611ba 100644
--- a/output/rhv-upload-precheck.py
+++ b/output/rhv-upload-precheck.py
@@ -18,6 +18,7 @@
import json
import logging
+import re
import sys
from urllib.parse import urlparse
@@ -46,6 +47,15 @@ output_password = output_password.rstrip()
parsed = urlparse(params['output_conn'])
username = parsed.username or "admin@internal"
+# Check the storage domain name is valid
+# (https://bugzilla.redhat.com/show_bug.cgi?id=1986386#c1)
+# Also this means it cannot contain spaces or glob symbols, so
+# the search below is valid.
+output_storage = params['output_storage']
+if not re.match('^[-a-zA-Z0-9_]+$', output_storage):
+raise RuntimeError("The storage domain (-os) parameter ‘%s’ is not valid" %
+ output_storage)
+
# Connect to the server.
connection = sdk.Connection(
url=params['output_conn'],
@@ -60,28 +70,33 @@ system_service = connection.system_service()
# Check whether there is a datacenter for the specified storage.
data_centers = system_service.data_centers_service().list(
-search='storage.name=%s' % params['output_storage'],
+search='storage.name=%s' % output_storage,
case_sensitive=True,
)
if len(data_centers) == 0:
storage_domains = system_service.storage_domains_service().list(
-search='name=%s' % params['output_storage'],
+search='name=%s' % output_storage,
case_sensitive=True,
)
if len(storage_domains) == 0:
# The storage domain does not even exist.
raise RuntimeError("The storage domain ‘%s’ does not exist" %
- (params['output_storage']))
+ output_storage)
# The storage domain is not attached to a datacenter
# (shouldn't happen, would fail on disk creation).
raise RuntimeError("The storage domain ‘%s’ is not attached to a DC" %
- (params['output_storage']))
+ output_storage)
datacenter = data_centers[0]
# Get the storage domain.
storage_domains = connection.follow_link(datacenter.storage_domains)
-storage_domain = [sd for sd in storage_domains if sd.name ==
params['output_storage']][0]
+try:
+storage_domain = [sd for sd in storage_domains
+ if sd.name == output_storage][0]
+except IndexError:
+raise RuntimeError("The storage domain ‘%s’ does not exist" %
+ output_storage)
# Get the cluster.
clusters = connection.follow_link(datacenter.clusters)
@@ -90,7 +105,7 @@ if len(clusters) == 0:
raise RuntimeError("The cluster ‘%s’ is not part of the DC ‘%s’, "
"where the storage domain ‘%s’ is" %
(params['rhv_cluster'], datacenter.name,
-params['output_storage']))
+output_storage))
cluster = clusters[0]
cpu = cluster.cpu
if cpu.architecture == types.Architecture.UNDEFINED:
--
2.39.0
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2v] -o rhv-upload: Give a nicer error if the storage domain does not exist
On Fri, Jan 27, 2023 at 1:18 PM Nir Soffer wrote:
>
> On Thu, Jan 26, 2023 at 2:31 PM Richard W.M. Jones wrote:
> >
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1986386
> > Reported-by: Junqin Zhou
> > ---
> > output/rhv-upload-precheck.py | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/output/rhv-upload-precheck.py b/output/rhv-upload-precheck.py
> > index 1dc1b8498a..35ea021032 100644
> > --- a/output/rhv-upload-precheck.py
> > +++ b/output/rhv-upload-precheck.py
> > @@ -81,7 +81,12 @@ datacenter = data_centers[0]
> >
> > # Get the storage domain.
> > storage_domains = connection.follow_link(datacenter.storage_domains)
> > -storage_domain = [sd for sd in storage_domains if sd.name ==
> > params['output_storage']][0]
> > +try:
> > +storage_domain = [sd for sd in storage_domains \
> > + if sd.name == params['output_storage']][0]
>
> Using `\` may work but it is needed. You can do this:
>
> storage_domain = [sd for sd in storage_domains
>if sd.name == params['output_storage']][0]
>
> This is also the common way to indent list comprehension that
> makes the expression more clear.
>
> > +except IndexError:
> > +raise RuntimeError("The storage domain ‘%s’ does not exist" %
> > + params['output_storage'])
>
> The fix is safe and makes sense.
>
> Not sure why we list all storage domains when we already know the name,
> maybe Albert would like to clean up this mess later.
Like this:
https://github.com/oVirt/python-ovirt-engine-sdk4/blob/2aa50266056b7ee0b72597f346cbf0f006041566/examples/list_storage_domains.py#L93
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2v] -o rhv-upload: Give a nicer error if the storage domain does not exist
On Thu, Jan 26, 2023 at 2:31 PM Richard W.M. Jones wrote:
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1986386
> Reported-by: Junqin Zhou
> ---
> output/rhv-upload-precheck.py | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/output/rhv-upload-precheck.py b/output/rhv-upload-precheck.py
> index 1dc1b8498a..35ea021032 100644
> --- a/output/rhv-upload-precheck.py
> +++ b/output/rhv-upload-precheck.py
> @@ -81,7 +81,12 @@ datacenter = data_centers[0]
>
> # Get the storage domain.
> storage_domains = connection.follow_link(datacenter.storage_domains)
> -storage_domain = [sd for sd in storage_domains if sd.name ==
> params['output_storage']][0]
> +try:
> +storage_domain = [sd for sd in storage_domains \
> + if sd.name == params['output_storage']][0]
Using `\` may work but it is needed. You can do this:
storage_domain = [sd for sd in storage_domains
if sd.name == params['output_storage']][0]
This is also the common way to indent list comprehension that
makes the expression more clear.
> +except IndexError:
> +raise RuntimeError("The storage domain ‘%s’ does not exist" %
> + params['output_storage'])
The fix is safe and makes sense.
Not sure why we list all storage domains when we already know the name,
maybe Albert would like to clean up this mess later.
Nir
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2v] -o rhv-upload: Give a nicer error if the storage domain
On Thu, Jan 26, 2023 at 2:31 PM Richard W.M. Jones wrote: > > https://bugzilla.redhat.com/show_bug.cgi?id=1986386 > > My RHV instance is dead at the moment so I didn't do much more than > check this compiles and passes the one test we have. Also I want to > spend as little time as possible on RHV outputs for virt-v2v since the > RHV product will be discontinued soon. > > I did want to point out some things: > > - The preceeding code is probably wrong. > > https://github.com/libguestfs/virt-v2v/blob/master/output/rhv-upload-transfer.py#L70 > >It attempts to search for the output storage using: > > storage_domains = system_service.storage_domains_service().list( > search='name=%s' % params['output_storage'], > case_sensitive=True, > ) I think the search is correct. This is explained in https://bugzilla.redhat.com/1986386#c1 >I couldn't find any documentation about what can go into that >search string, but it's clearly a lot more complicated than just >pasting in the literal name after "name=". At the very least, >spaces are not permitted, see: > > https://github.com/libguestfs/virt-v2v/blob/master/output/rhv-upload-transfer.py#L70 True, search can be an expression. > - The bug reporter used "data*" as the name and I suspect that is >parsed in some way (wildcard? regexp? I've no idea). It is treated as glob pattern, also explained in comment 1. > - Probably for the same reason, the preceeding code ought to fail >with an error if the output storage domain doesn't exist. The fact >we reach the code patched here at all also indicates some bug, >maybe in the search string. > > As I say above, I don't especially care about any of this. I'm not working on RHV since August 2022. Adding Albert who is current RHV storage maintainer. Nir ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
