Laszlo Ersek
2022-Mar-24 08:49 UTC
[Libguestfs] [PATCH p2v] ssh: Allow interop with virt-v2v 2.0
On 03/23/22 18:04, Richard W.M. Jones wrote:> The code previously checked that the remote version of virt-v2v was > 1.x and >= 1.28. Since virt-v2v 2.0 has been released and is broadly > compatible, allow 2.0 as well. > --- > ssh.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/ssh.c b/ssh.c > index ce0caaa..7ac43eb 100644 > --- a/ssh.c > +++ b/ssh.c > @@ -1030,11 +1030,11 @@ add_output_driver (const char *name, size_t len) > static int > compatible_version (const char *v2v_version) > { > - unsigned v2v_minor; > + unsigned v2v_major, v2v_minor; > > - /* The major version must always be 1. */ > - if (!STRPREFIX (v2v_version, "1.")) { > - set_ssh_error ("virt-v2v major version is not 1 (\"%s\"), " > + /* The major version must always be 1 or 2. */ > + if (! (STRPREFIX (v2v_version, "1.") || STRPREFIX (v2v_version, "2."))) { > + set_ssh_error ("virt-v2v major version is not 1 or 2 (\"%s\"), " > "this version of virt-p2v is not compatible.", > v2v_version); > return 0;I have a personal preference for avoiding such "outermost" logical negations; I always move negations as far inside as possible, using De Morgan's Laws: !STRPREFIX (v2v_version, "1.") && !STRPREFIX (v2v_version, "2.") However, this is just a style question, and the logic is certainly correct. Somewhat relatedly, regarding the English language error message, I think we could slightly clarify it: virt-v2v major version is neither 1 nor 2 The rest looks OK to me. With or without the msg update: Reviewed-by: Laszlo Ersek <lersek at redhat.com> Thanks Laszlo> @@ -1045,13 +1045,13 @@ compatible_version (const char *v2v_version) > * that we published during development, nor (b) using old virt-v2v. > * We should remain compatible with any virt-v2v after 1.28. > */ > - if (sscanf (v2v_version, "1.%u", &v2v_minor) != 1) { > + if (sscanf (v2v_version, "%u.%u", &v2v_major, &v2v_minor) != 2) { > set_ssh_internal_error ("cannot parse virt-v2v version string (\"%s\")", > v2v_version); > return 0; > } > > - if (v2v_minor < 28) { > + if (v2v_major == 1 && v2v_minor < 28) { > set_ssh_error ("virt-v2v version is < 1.28 (\"%s\"), " > "you must upgrade to virt-v2v >= 1.28 on " > "the conversion server.", v2v_version); >
Richard W.M. Jones
2022-Mar-24 09:13 UTC
[Libguestfs] [PATCH p2v] ssh: Allow interop with virt-v2v 2.0
Thanks - pushed that as commit 546f3a6. While I remember, there's another odd thing about virt-p2v that could be simplified if you're looking for some general clean-up work to do to get used to the code. virt-p2v needs an NBD server running on the p2v machine to export the disks. Originally we used qemu-nbd since that was the only choice. In 2017, I added support for nbdkit as an alternative to qemu-nbd. So now we're in the situation where either server can be used (see virt-p2v.git/nbd.c). For added complexity we also support both servers in either systemd socket activation (SA) mode or "no-SA" mode, so that's 4 combinations. This is silly, we should support only one NBD server, and since systemd socket activation is well-supported and more flexible, we should just use it. So the question is *which* NBD server to support. That's not so much a technical matter since both servers can easily serve a local block device (always raw format). However I do think that nbdkit might genuinely be the better choice here: - qemu-nbd links to the whole qemu block layer, nbdkit can be shipped with just the plugin we need, so it should be smaller with less code surface - nbdkit-file-plugin has a better method of not trashing the host page cache - could use nbdkit --exit-with-parent feature (which we don't at the moment) - nbdkit is widely available in distros these days Also that file uses AI_ADDRCONFIG so I guess it has problems with IPv6. 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