Richard W.M. Jones
2023-Jul-14 09:40 UTC
[Libguestfs] [libguestfs PATCH 2/7] lib/launch-libvirt: support networking with passt
On Thu, Jul 13, 2023 at 07:10:47PM +0200, Laszlo Ersek wrote:> We generate the <interface type="user"> element on libvirt 3.8.0+ already. > > For selecting passt rather than SLIRP, we only need to insert the child > element <backend type='passt'>. Make that child element conditional on > libvirt 9.0.0+, plus "passt --help" being executable. > > For the latter, place the new helper function guestfs_int_passt_runnable() > in "lib/launch.c" -- we're going to use the same function for the direct > backend as well. > > This change exposes a number of (perceived) shortcomings in libvirt; I've > filed <https://bugzilla.redhat.com/show_bug.cgi?id=2222766> about those. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > --- > lib/guestfs-internal.h | 1 + > lib/launch-libvirt.c | 11 ++++++++ > lib/launch.c | 28 ++++++++++++++++++++ > 3 files changed, 40 insertions(+) > > diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h > index 4be351e3d3cc..a6c4266ad3fe 100644 > --- a/lib/guestfs-internal.h > +++ b/lib/guestfs-internal.h > @@ -703,6 +703,7 @@ extern void guestfs_int_unblock_sigterm (void); > extern int guestfs_int_create_socketname (guestfs_h *g, const char *filename, char (*sockname)[UNIX_PATH_MAX]); > extern void guestfs_int_register_backend (const char *name, const struct backend_ops *); > extern int guestfs_int_set_backend (guestfs_h *g, const char *method); > +extern bool guestfs_int_passt_runnable (guestfs_h *g); > > /* Close all file descriptors matching the condition. */ > #define close_file_descriptors(cond) do { \ > diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c > index d4bf1a8ff242..994909a35f2d 100644 > --- a/lib/launch-libvirt.c > +++ b/lib/launch-libvirt.c > @@ -1414,6 +1414,17 @@ construct_libvirt_xml_devices (guestfs_h *g, > guestfs_int_version_ge (¶ms->data->libvirt_version, 3, 8, 0)) { > start_element ("interface") { > attribute ("type", "user"); > + /* If libvirt is 9.0.0+ and "passt" is available, ask for passt rather > + * than SLIRP (RHBZ#2184967). Note that this causes some > + * appliance-visible changes (although network connectivity is certainly > + * functional); refer to RHBZ#2222766 about those. > + */ > + if (guestfs_int_version_ge (¶ms->data->libvirt_version, 9, 0, 0) && > + guestfs_int_passt_runnable (g)) { > + start_element ("backend") { > + attribute ("type", "passt"); > + } end_element (); > + } > start_element ("model") { > attribute ("type", "virtio"); > } end_element (); > diff --git a/lib/launch.c b/lib/launch.c > index 6e08b12006da..94c8f676d8bd 100644 > --- a/lib/launch.c > +++ b/lib/launch.c > @@ -36,6 +36,7 @@ > #include <signal.h> > #include <sys/stat.h> > #include <sys/types.h> > +#include <sys/wait.h> > #include <errno.h> > #include <assert.h> > #include <libintl.h> > @@ -414,6 +415,33 @@ guestfs_int_set_backend (guestfs_h *g, const char *method) > return 0; > } > > +/** > + * Return C<true> if we can call S<C<passt --help>>, and it exits with status > + * C<0> or C<1>. > + * > + * (At least C<passt-0^20230222.g4ddbcb9-2.el9_2.x86_64> terminates with status > + * C<1> in response to "--help", which is arguably wrong, and potentially > + * subject to change, but it doesn't really matter.) > + */ > +bool > +guestfs_int_passt_runnable (guestfs_h *g) > +{ > + CLEANUP_CMD_CLOSE struct command *cmd = NULL; > + int r, ex; > + > + cmd = guestfs_int_new_command (g); > + if (cmd == NULL) > + return false; > + > + guestfs_int_cmd_add_string_unquoted (cmd, "passt --help");Do we need " >/dev/null 2>&1" here to avoid unnecessary messages being printed when libguestfs is not in verbose mode? Apart from that: Reviewed-by: Richard W.M. Jones <rjones at redhat.com> Rich.> + r = guestfs_int_cmd_run (cmd); > + if (r == -1 || !WIFEXITED (r)) > + return false; > + > + ex = WEXITSTATUS (r); > + return ex == 0 || ex == 1; > +} > + > /* This hack is only required to make static linking work. See: > * https://stackoverflow.com/questions/1202494/why-doesnt-attribute-constructor-work-in-a-static-library > */ > > _______________________________________________ > Libguestfs mailing list > Libguestfs at redhat.com > https://listman.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-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
Laszlo Ersek
2023-Jul-14 10:18 UTC
[Libguestfs] [libguestfs PATCH 2/7] lib/launch-libvirt: support networking with passt
On 7/14/23 11:40, Richard W.M. Jones wrote:> On Thu, Jul 13, 2023 at 07:10:47PM +0200, Laszlo Ersek wrote: >> We generate the <interface type="user"> element on libvirt 3.8.0+ already. >> >> For selecting passt rather than SLIRP, we only need to insert the child >> element <backend type='passt'>. Make that child element conditional on >> libvirt 9.0.0+, plus "passt --help" being executable. >> >> For the latter, place the new helper function guestfs_int_passt_runnable() >> in "lib/launch.c" -- we're going to use the same function for the direct >> backend as well. >> >> This change exposes a number of (perceived) shortcomings in libvirt; I've >> filed <https://bugzilla.redhat.com/show_bug.cgi?id=2222766> about those. >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967 >> Signed-off-by: Laszlo Ersek <lersek at redhat.com> >> --- >> lib/guestfs-internal.h | 1 + >> lib/launch-libvirt.c | 11 ++++++++ >> lib/launch.c | 28 ++++++++++++++++++++ >> 3 files changed, 40 insertions(+) >> >> diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h >> index 4be351e3d3cc..a6c4266ad3fe 100644 >> --- a/lib/guestfs-internal.h >> +++ b/lib/guestfs-internal.h >> @@ -703,6 +703,7 @@ extern void guestfs_int_unblock_sigterm (void); >> extern int guestfs_int_create_socketname (guestfs_h *g, const char *filename, char (*sockname)[UNIX_PATH_MAX]); >> extern void guestfs_int_register_backend (const char *name, const struct backend_ops *); >> extern int guestfs_int_set_backend (guestfs_h *g, const char *method); >> +extern bool guestfs_int_passt_runnable (guestfs_h *g); >> >> /* Close all file descriptors matching the condition. */ >> #define close_file_descriptors(cond) do { \ >> diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c >> index d4bf1a8ff242..994909a35f2d 100644 >> --- a/lib/launch-libvirt.c >> +++ b/lib/launch-libvirt.c >> @@ -1414,6 +1414,17 @@ construct_libvirt_xml_devices (guestfs_h *g, >> guestfs_int_version_ge (¶ms->data->libvirt_version, 3, 8, 0)) { >> start_element ("interface") { >> attribute ("type", "user"); >> + /* If libvirt is 9.0.0+ and "passt" is available, ask for passt rather >> + * than SLIRP (RHBZ#2184967). Note that this causes some >> + * appliance-visible changes (although network connectivity is certainly >> + * functional); refer to RHBZ#2222766 about those. >> + */ >> + if (guestfs_int_version_ge (¶ms->data->libvirt_version, 9, 0, 0) && >> + guestfs_int_passt_runnable (g)) { >> + start_element ("backend") { >> + attribute ("type", "passt"); >> + } end_element (); >> + } >> start_element ("model") { >> attribute ("type", "virtio"); >> } end_element (); >> diff --git a/lib/launch.c b/lib/launch.c >> index 6e08b12006da..94c8f676d8bd 100644 >> --- a/lib/launch.c >> +++ b/lib/launch.c >> @@ -36,6 +36,7 @@ >> #include <signal.h> >> #include <sys/stat.h> >> #include <sys/types.h> >> +#include <sys/wait.h> >> #include <errno.h> >> #include <assert.h> >> #include <libintl.h> >> @@ -414,6 +415,33 @@ guestfs_int_set_backend (guestfs_h *g, const char *method) >> return 0; >> } >> >> +/** >> + * Return C<true> if we can call S<C<passt --help>>, and it exits with status >> + * C<0> or C<1>. >> + * >> + * (At least C<passt-0^20230222.g4ddbcb9-2.el9_2.x86_64> terminates with status >> + * C<1> in response to "--help", which is arguably wrong, and potentially >> + * subject to change, but it doesn't really matter.) >> + */ >> +bool >> +guestfs_int_passt_runnable (guestfs_h *g) >> +{ >> + CLEANUP_CMD_CLOSE struct command *cmd = NULL; >> + int r, ex; >> + >> + cmd = guestfs_int_new_command (g); >> + if (cmd == NULL) >> + return false; >> + >> + guestfs_int_cmd_add_string_unquoted (cmd, "passt --help"); > > Do we need " >/dev/null 2>&1" here to avoid unnecessary messages being > printed when libguestfs is not in verbose mode?Yes. Thanks for pointing it out. I didn't quite know what to do with the stdout / stderr of "passt --help". So is the following pattern the right one? guestfs_int_cmd_add_string_unquoted (cmd, "passt --help"); if (!g->verbose) guestfs_int_cmd_add_string_unquoted (cmd, " >/dev/null 2>&1");> > Apart from that: > > Reviewed-by: Richard W.M. Jones <rjones at redhat.com>Thanks! Laszlo> > Rich. > >> + r = guestfs_int_cmd_run (cmd); >> + if (r == -1 || !WIFEXITED (r)) >> + return false; >> + >> + ex = WEXITSTATUS (r); >> + return ex == 0 || ex == 1; >> +} >> + >> /* This hack is only required to make static linking work. See: >> * https://stackoverflow.com/questions/1202494/why-doesnt-attribute-constructor-work-in-a-static-library >> */ >> >> _______________________________________________ >> Libguestfs mailing list >> Libguestfs at redhat.com >> https://listman.redhat.com/mailman/listinfo/libguestfs >