Libguestfs - Jun 2022 - [v2v PATCH 0/4] convert_linux: install the QEMU guest agent with a firstboot script

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764

I'm going to post the pre-requisite libguestfs-common and guestfs-tools
patches (one patch for each project) in response to this cover letter,
too.

I'm not sure why we want to perform the installation specifically at
*firstboot* <https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c2>.
Virt-v2v already only supports conversions where host and guest arches
are identical; thus, the appliance kernel could run native guest
binaries (if necessary). I've read
<https://libguestfs.org/guestfs.3.html#running-commands>, but I'm not
overly convinced. Firstboot comes with *lots* of complications. Even
<https://libguestfs.org/virt-builder.1.html#installing-packages>
mentions some of them:
> The downsides are that it will take the guest a lot longer to boot
> first time, and there?s nothing much you can do if package
> installation fails (eg. if a network problem means the guest can't
> reach the package repositories).
Anyway, here goes.

Thanks,
Laszlo

Laszlo Ersek (4):
  output/create_libvirt_xml: wire up the QEMU guest agent
  windows_virtio: remove "install_linux_tools"
  convert_linux: extract qemu-guest-agent package name
  convert_linux: install the QEMU guest agent with a firstboot script

 common                       |   2 +-
 convert/convert_linux.ml     | 102 ++++++++++++++++++--
 convert/linux.ml             |  35 -------
 convert/linux.mli            |  11 ---
 convert/windows_virtio.ml    |  42 --------
 convert/windows_virtio.mli   |   4 -
 output/create_libvirt_xml.ml |  11 +++
 tests/test-v2v-i-ova.xml     |   4 +
 8 files changed, 111 insertions(+), 100 deletions(-)

-- 
2.19.1.3.g30247aa5d201

The intent (even before RHBZ#2028764) has been to install the QEMU guest
agent in the converted domain unconditionally. Therefore, in order for the
GA to be actually accessible from the host side, augment the libvirt
output module with a "guest agent connection" also unconditionally.

For starters, the domain needs a virtio-serial device. Then there must be
a port on the device that (in the guest) the GA identifies by name, and
that (on the host) is exposed as a listening socket (usually in the unix
address family). The adress of that port (usually a pathname, i.e., for a
unix domain socket) is then passed to whatever host-side application wants
to talk to the GA.

The minimal domain XML fragment for that ("minimal" for our purposes)
is

  <controller type='virtio-serial' model='virtio'>
  <channel type='unix'>
    <target type='virtio' name='org.qemu.guest_agent.0'/>
  </channel>

The "controller" element is needed because
"controller/@model" is where we
regulate "virtio" vs. "virtio-transitional".

Everything else is filled in by libvirt. Notably, libvirt (a) creates and
binds the unix domain socket itself (usually
"/var/lib/libvirt/qemu/channel/target/DOMAIN/org.qemu.guest_agent.0"),
(b)
passes the file descriptor to QEMU, and (c) figures out the socket
pathname for commands such as

  virsh domfsinfo DOMAIN
  virsh domhostname DOMAIN --source agent
  virsh domifaddr DOMAIN --source agent
  virsh guestinfo DOMAIN

For QEMU, the corresponding options would be

  -chardev socket,id=agent,server=on,wait=off,path=/tmp/DOMAIN-agent \
  -device virtio-serial-pci,id=vioserial \
  -device
virtserialport,bus=vioserial.0,nr=1,chardev=agent,name=org.qemu.guest_agent.0 \

Note the "path=/tmp/DOMAIN-agent" property of "-chardev";
virt-v2v would
have to generate that (in place of the "fd=nnnn" property that libvirt
passes to QEMU).

Omit extending the QEMU output module for now, as the QGA protocol is
based on JSON, and one needs "virsh" or "virt-manager" (or
another
management application interface) anyway, for efficiently exchanging
messages with QGA. I don't know of end-user tools that directly connect to
"/tmp/DOMAIN-agent".

Don't modify the RHV and OpenStack outputs either; both of these
management products likely configure the virtio-serial device
automatically, for the agent access.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 output/create_libvirt_xml.ml | 11 +++++++++++
 tests/test-v2v-i-ova.xml     |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
index 68d0a90918f3..531a4f75bf3e 100644
--- a/output/create_libvirt_xml.ml
+++ b/output/create_libvirt_xml.ml
@@ -524,6 +524,17 @@ let create_libvirt_xml ?pool source inspect
     e "console" ["type", "pty"] [];
   ];
 
+  (* Given that we install the QEMU Guest Agent for both Linux and Windows
+   * guests unconditionally, create the virtio-serial device that's needed
for
+   * communication between the host and the agent.
+   *)
+  List.push_back_list devices [
+    e "controller" ["type", "virtio-serial";
"model", virtio_model] [];
+    e "channel" ["type", "unix"] [
+      e "target" ["type", "virtio";
"name", "org.qemu.guest_agent.0"] []
+    ]
+  ];
+
   List.push_back_list body [
     e "devices" [] !devices;
   ];
diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
index 6b8cda62f051..da1db473e53c 100644
--- a/tests/test-v2v-i-ova.xml
+++ b/tests/test-v2v-i-ova.xml
@@ -49,5 +49,9 @@
     <input type='tablet' bus='usb'/>
     <input type='mouse' bus='ps2'/>
     <console type='pty'/>
+    <controller type='virtio-serial' model='virtio'/>
+    <channel type='unix'>
+      <target type='virtio'
name='org.qemu.guest_agent.0'/>
+    </channel>
   </devices>
 </domain>
-- 
2.19.1.3.g30247aa5d201

"Windows_virtio.install_linux_tools" has never really worked in
practice;
we've never managed to get the right content into the right location on
the virtio-win ISO.

Later patches in this series will install the qemu guest agent in a
firstboot script, using the guest's own package manager. For now, for ease
of review, only remove "Windows_virtio.install_linux_tools", and its
dependencies that now become unused:

- Linux.architecture_string
- Linux.binary_package_extension
- Linux.install_local

Salvage the comment (at the outermost call site) that failure to install
QGA is not fatal. This will be relevant for exception handling in the
subsequent patches.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 convert/linux.mli          | 11 -----
 convert/windows_virtio.mli |  4 --
 convert/convert_linux.ml   |  4 +-
 convert/linux.ml           | 35 ----------------
 convert/windows_virtio.ml  | 42 --------------------
 5 files changed, 3 insertions(+), 93 deletions(-)

diff --git a/convert/linux.mli b/convert/linux.mli
index 856ffe3c95ab..57898310597c 100644
--- a/convert/linux.mli
+++ b/convert/linux.mli
@@ -23,9 +23,6 @@ val augeas_reload : Guestfs.guestfs -> unit
     additional debugging information about parsing problems
     that augeas found. *)
 
-val install_local: Guestfs.guestfs -> Types.inspect -> string list ->
unit
-(** Install package(s). *)
-
 val remove : Guestfs.guestfs -> Types.inspect -> string list -> unit
 (** Uninstall package(s). *)
 
@@ -38,11 +35,3 @@ val is_file_owned : Guestfs.guestfs -> Types.inspect ->
string -> bool
 val is_package_manager_save_file : string -> bool
 (** Return true if the filename is something like [*.rpmsave], ie.
     a package manager save-file. *)
-
-val binary_package_extension : Types.inspect -> string
-(** Return the extension typically used for binary packages in the
-    specified package format. *)
-
-val architecture_string : Types.inspect -> string
-(** Return the architecture string typically used for binary packages
-    in the specified package format, and for the specified distro. *)
diff --git a/convert/windows_virtio.mli b/convert/windows_virtio.mli
index a92cc01da6df..73ec95c71839 100644
--- a/convert/windows_virtio.mli
+++ b/convert/windows_virtio.mli
@@ -38,10 +38,6 @@ val install_drivers
     reflecting what devices are now required by the guest, either virtio
     devices if we managed to install those, or legacy devices if we didn't.
*)
 
-val install_linux_tools : Guestfs.guestfs -> Types.inspect -> unit
-(** installs QEMU Guest Agent on Linux guest OS from the driver directory or
-    driver ISO. It is not fatal if we fail to install the agent. *)
-
 val copy_qemu_ga : Guestfs.guestfs -> Types.inspect -> string list
 (** copy MSIs (idealy just one) with QEMU Guest Agent to Windows guest. The
     MSIs are not installed by this function. *)
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index 5660494973b1..79462aa1e7ae 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -538,13 +538,15 @@ let convert (g : G.guestfs) source inspect
keep_serial_console _      )
 
   and install_linux_tools () +    (* It is not fatal if we fail to install the
QEMU guest agent. *)
     let has_qemu_guest_agent        List.exists (
         fun { G.app2_name = name } ->
           name = "qemu-guest-agent"
       ) inspect.i_apps in
     if not has_qemu_guest_agent then
-      Windows_virtio.install_linux_tools g inspect
+      (* FIXME -- install qemu-guest-agent here *)
+      ()
 
   and configure_kernel ()      (* Previously this function would try to install
kernels, but we
diff --git a/convert/linux.ml b/convert/linux.ml
index e2908d02be28..f9acd63f9b82 100644
--- a/convert/linux.ml
+++ b/convert/linux.ml
@@ -33,20 +33,6 @@ let augeas_reload g    g#aug_load ();
   debug_augeas_errors g
 
-let rec install_local g { i_package_format = package_format } packages -  if
packages <> [] then (
-    match package_format with
-    | "rpm" ->
-      let cmd = [ "rpm"; "--upgrade"; "-v" ] @
packages in
-      let cmd = Array.of_list cmd in
-      ignore (g#command cmd)
-    | format ->
-      error (f_"don?t know how to install packages using %s: packages:
%s")
-        format (String.concat " " packages)
-    (* Reload Augeas in case anything changed. *)
-    augeas_reload g
-  )
-
 let rec remove g inspect packages    if packages <> [] then (
     do_remove g inspect packages;
@@ -187,24 +173,3 @@ let is_package_manager_save_file filename    (* Recognized
suffixes of package managers. *)
   let suffixes = [ ".dpkg-old"; ".dpkg-new";
".rpmsave"; ".rpmnew"; ] in
   List.exists (Filename.check_suffix filename) suffixes
-
-let binary_package_extension { i_package_format = package_format } -  match
package_format with
-  | "deb" -> "deb"
-  | "rpm" -> "rpm"
-  | format ->
-    error (f_"don?t know what is the extension of binary packages using
%s")
-      format
-
-let architecture_string { i_package_format = package_format; i_arch = arch;
-                          i_distro = distro } -  match package_format, distro,
arch with
-  | "deb", _, "x86_64" -> "amd64"
-  | "deb", _, a -> a
-  | "rpm",
("sles"|"suse-based"|"opensuse"), "i386"
-> "i586"
-  | "rpm",
("sles"|"suse-based"|"opensuse"), a -> a
-  | "rpm", _, "i386" -> "i686"
-  | "rpm", _, a -> a
-  | format, distro, arch ->
-    error (f_"don?t know what is the architecture string of %s using %s on
%s")
-      arch format distro
diff --git a/convert/windows_virtio.ml b/convert/windows_virtio.ml
index 5254322c7d28..a27cd6a5432e 100644
--- a/convert/windows_virtio.ml
+++ b/convert/windows_virtio.ml
@@ -113,48 +113,6 @@ let rec install_drivers ((g, _) as reg) inspect      
virtio_rng_supported, virtio_ballon_supported, isa_pvpanic_supported,
virtio_socket_supported)
   )
 
-and install_linux_tools g inspect -  let os -    match inspect.i_distro with
-    | "fedora" -> Some "fc28"
-    | "rhel" | "centos" | "scientificlinux" |
"redhat-based"
-    | "oraclelinux" ->
-       (* map 6 -> "el6" etc. *)
-       if inspect.i_major_version >= 6 then
-         Some (sprintf "el%d" inspect.i_major_version)
-       else
-         None
-    | "sles" | "suse-based" | "opensuse" ->
Some "lp151"
-    | _ -> None in
-
-  match os with
-  | None -> ()
-  | Some os ->
-      let src_path = "linux" // os in
-      let dst_path = "/var/tmp" in
-      let pkg_arch = Linux.architecture_string inspect in
-      let pkg_ext = Linux.binary_package_extension inspect in
-      let package_suffixes = [
-        sprintf ".%s.%s" pkg_arch pkg_ext;
-        sprintf "_%s.%s" pkg_arch pkg_ext;
-      ] in
-      let package_filter path _ -        List.exists (String.is_suffix path)
package_suffixes
-      in
-      debug "locating packages in %s" src_path;
-      let packages -        copy_from_virtio_win g inspect src_path dst_path
-                             package_filter
-                             (fun () -> ()) in
-      debug "done copying %d files" (List.length packages);
-      let packages = List.map ((//) dst_path) packages in
-      try
-        Linux.install_local g inspect packages;
-        if packages <> [] then
-          info (f_"QEMU Guest Agent installed for this guest.");
-      with G.Error msg ->
-        warning (f_"failed to install QEMU Guest Agent: %s") msg
-
 and add_guestor_to_registry ((g, root) as reg) inspect drv_name drv_pciid   
let ddb_node = g#hivex_node_get_child root "DriverDatabase" in
 
-- 
2.19.1.3.g30247aa5d201

In commit a30383e35d34 ("v2v: linux: do not install qemu-guest-agent if
already installed", 2019-09-20), the name of the package providing the
QEMU guest agent was hard-coded as "qemu-guest-agent", regardless of
distro family. Turns out this is actually correct (and may have been
intentional, only it was not specifically documented): in all OS families
currently recognized by our "family" function (`RHEL_family,
`ALT_family,
`SUSE_family, `Debian_family), the *binary* package is indeed called
"qemu-guest-agent":

- https://brewweb.engineering.redhat.com/brew/packageinfo?packageID=47646
-
http://rpmfind.net/linux/rpm2html/search.php?query=qemu-guest-agent&submit=Search+...&system=&arch-
https://packages.altlinux.org/en/sisyphus/srpms/qemu/
-
https://packages.debian.org/search?keywords=qemu-guest-agent&searchon=names&suite=all&section=all

As a way of documenting this, extract the mapping to a new helper function
named "qga_pkg_of_family".

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 convert/convert_linux.ml | 33 +++++++++++++++-----
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index 79462aa1e7ae..2ddbc07aa86a 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -56,6 +56,16 @@ let convert (g : G.guestfs) source inspect
keep_serial_console _      | "debian" | "ubuntu" |
"linuxmint" | "kalilinux" -> `Debian_family
     | _ -> assert false in
 
+  (* map the OS family name to the qemu-guest-agent package name *)
+  let qga_pkg_of_family +    function
+    | `RHEL_family
+    | `ALT_family
+    | `SUSE_family
+    | `Debian_family -> Some "qemu-guest-agent"
+    | _ -> None
+  in
+
   assert (inspect.i_package_format = "rpm" ||
inspect.i_package_format = "deb");
 
   (* Fail early if i_apps is empty.  Certain steps such as kernel
@@ -539,14 +549,21 @@ let convert (g : G.guestfs) source inspect
keep_serial_console _
   and install_linux_tools ()      (* It is not fatal if we fail to install the
QEMU guest agent. *)
-    let has_qemu_guest_agent -      List.exists (
-        fun { G.app2_name = name } ->
-          name = "qemu-guest-agent"
-      ) inspect.i_apps in
-    if not has_qemu_guest_agent then
-      (* FIXME -- install qemu-guest-agent here *)
-      ()
+    match qga_pkg_of_family family with
+    | None -> warning (f_"The name of the package that provides the
QEMU Guest \
+                          Agent for this guest OS is unknown.  The guest agent
\
+                          will not be installed.  Please consider reporting a \
+                          bug according to the BUGS section of the virt-v2v(1)
\
+                          manual.")
+    | Some qga_pkg ->
+        let has_qemu_guest_agent +          List.exists (
+            fun { G.app2_name = name } ->
+              name = qga_pkg
+          ) inspect.i_apps in
+        if not has_qemu_guest_agent then
+          (* FIXME -- install qemu-guest-agent here *)
+          ()
 
   and configure_kernel ()      (* Previously this function would try to install
kernels, but we
-- 
2.19.1.3.g30247aa5d201

Register a firstboot script, for installing the guest agent with the
guest's own package manager -- that is,
"Guest_packages.install_command".

For installing the package, network connectivity is required; for lack of
a universal "wait online" dependency expression, register "sleep
60" as a
preceding firstboot script.

The source domain's SELinux policy may not allow our firstboot service to
execute the package's installation scripts (if any). For that reason,
temporarily disable SELinux around package installation.

After installation, register another script for launching the agent.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 convert/convert_linux.ml | 73 +++++++++++++++++++-
 common                   |  2 +-
 2 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index 2ddbc07aa86a..1dcd7abbcee2 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -562,8 +562,77 @@ let convert (g : G.guestfs) source inspect
keep_serial_console _                name = qga_pkg
           ) inspect.i_apps in
         if not has_qemu_guest_agent then
-          (* FIXME -- install qemu-guest-agent here *)
-          ()
+          let pkg_mgmt = g#inspect_get_package_management inspect.i_root in
+          try
+            let inst_cmd = Guest_packages.install_command [qga_pkg] pkg_mgmt in
+
+            (* Use only the portable filename character set in this. *)
+            let selinux_enforcing =
"/root/virt-v2v-fb-selinux-enforcing" in
+            let fbs +              Firstboot.add_firstboot_script g
inspect.i_root
+            in
+            info (f_"The QEMU Guest Agent will be installed for this guest
at \
+                     first boot.");
+
+            (* Waiting for the guest to go "online" is very
complicated. On
+             * systemd-based distros, our firstboot service could perhaps
depend
+             * on "systemd-networkd-wait-online.service" or
+             * "NetworkManager-wait-online.service". But not all
Linux distros
+             * are based on systemd, and even with systemd, a guest could use
+             * systemd-networkd vs. NetworkManager... So just do what we do in
+             * various Windows firstboot scripts: wait. We could cut the
waiting
+             * short by pinging a well-known IP address, or by resolving a
+             * well-known FQDN, but those might qualify as "phoning
home"... So
+             * just wait.
+             *)
+            fbs "wait online"
+              "#!/bin/sh\n\
+               sleep 60\n";
+
+            (* Disable SELinux temporarily around package installation. Refer
to
+             * <https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c7>
and
+             * <https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c8>.
+             *)
+            fbs "setenforce 0"
+              (sprintf "#!/bin/sh\n\
+                        rm -f %s\n\
+                        if command -v getenforce >/dev/null &&\n\
+                        \ \ test Enforcing = \"$(getenforce)\"\n\
+                        then\n\
+                        \ \ touch %s\n\
+                        \ \ setenforce 0\n\
+                        fi\n" selinux_enforcing selinux_enforcing);
+            fbs "install qga" inst_cmd;
+            fbs "setenforce restore"
+              (sprintf "#!/bin/sh\n\
+                        if test -f %s; then\n\
+                        \ \ setenforce 1\n\
+                        \ \ rm -f %s\n\
+                        fi\n" selinux_enforcing selinux_enforcing);
+
+            (* Start the agent now and at subsequent boots. The following
+             * commands should work on both sysvinit distros / distro versions
+             * (regardless of "/etc/rc.d/" vs.
"/etc/init.d/" being the scheme
+             * in use) and systemd distros (via redirection to systemctl).
+             *
+             * On distros where the chkconfig command is redirected to
+             * systemctl, the chkconfig command is likely superfluous.
That's
+             * because on systemd distros, the QGA package comes with such
+             * runtime dependencies / triggers that the presence of the
+             * virtio-serial port named "org.qemu.guest_agent.0"
automatically
+             * starts the agent during (second and later) boots. However, even
+             * on such distros, the chkconfig command should do no harm.
+             *)
+            fbs "start qga"
+              (sprintf "#!/bin/sh\n\
+                        service %s start\n\
+                        chkconfig %s on\n" qga_pkg qga_pkg)
+          with
+          | Guest_packages.Unknown_package_manager msg
+          | Guest_packages.Unimplemented_package_manager msg ->
+            warning (f_"The QEMU Guest Agent will not be installed.  The \
+                        install command for package ?%s? could not be created:
\
+                        %s.") qga_pkg msg
 
   and configure_kernel ()      (* Previously this function would try to install
kernels, but we
diff --git a/common b/common
index 48527b8768d7..79828d1a92a4 160000
--- a/common
+++ b/common
@@ -1 +1 @@
-Subproject commit 48527b8768d7e010552beb62500a46ad940bca9a
+Subproject commit 79828d1a92a407edd8f946db5cc1bf07d260bf2f
-- 
2.19.1.3.g30247aa5d201

Factor the following internal functions from "guestfs-tools/customize"
at
commit 40b28512f700 ("Version 1.49.2.", 2022-05-26):

- guest_install_command
- guest_update_command
- guest_uninstall_command

into a new interface in "libguestfs-common/mlcustomize".

Keep the "error_unknown_package_manager" and
"error_unimplemented_package_manager" helpers hidden from the
interface.
Rewrap their message strings. Move them to the front, and make them raise
new exception types rather than exiting directly with
"Tools_utils.error".

Strip the "guest_" prefix from the exposed functions' names, as
external
referrers will qualify the functions with the "Guest_packages"
interface
name anyway.

In all three functions, replace the common

  g#inspect_get_package_management root

call (wherein the original context provides both "g" and
"root") with a
new parameter that is the result of that call; name it
"package_management".

Write new documentation for the interface.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 mlcustomize/Makefile.am        |   2 +
 mlcustomize/guest_packages.mli |  44 +++++++
 mlcustomize/guest_packages.ml  | 132 ++++++++++++++++++++
 3 files changed, 178 insertions(+)

diff --git a/mlcustomize/Makefile.am b/mlcustomize/Makefile.am
index cd7d8971ee06..4e2606471136 100644
--- a/mlcustomize/Makefile.am
+++ b/mlcustomize/Makefile.am
@@ -38,10 +38,12 @@ generator_built = \
 
 SOURCES_MLI = \
 	firstboot.mli \
+	guest_packages.mli \
 	SELinux_relabel.mli
 
 SOURCES_ML = \
 	firstboot.ml \
+	guest_packages.ml \
 	SELinux_relabel.ml
 
 if HAVE_OCAML
diff --git a/mlcustomize/guest_packages.mli b/mlcustomize/guest_packages.mli
new file mode 100644
index 000000000000..7504a6ab4d3e
--- /dev/null
+++ b/mlcustomize/guest_packages.mli
@@ -0,0 +1,44 @@
+(* virt-customize
+ * Copyright (C) 2012-2021 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *)
+
+exception Unknown_package_manager of string
+exception Unimplemented_package_manager of string
+(** For all three functions below, [package_management] determines the package
+    management system in use by the guest; commonly it should be filled in from
+    [Guestfs.inspect_get_package_management], or the equivalent guestfs object
+    method.
+
+    If [package_management] is unknown or unimplemented, the functions raise
+    [Unknown_package_manager "error message"] or
[Unimplemented_package_manager
+    "error message"], correspondingly. *)
+
+val install_command : string list -> string -> string
+(** [install_command packages package_management] produces a properly quoted
+    shell command string suitable for execution in the guest (directly or via a
+    Firstboot script) for installing the OS packages listed in [packages]. *)
+
+val update_command : string -> string
+(** [update_command package_management] produces a properly quoted shell
command
+    string suitable for execution in the guest (directly or via a Firstboot
+    script) for updating the OS packages that are currently installed in the
+    guest. *)
+
+val uninstall_command : string list -> string -> string
+(** [uninstall_command packages package_management] produces a properly quoted
+    shell command string suitable for execution in the guest (directly or via a
+    Firstboot script) for uninstalling the OS packages listed in [packages]. *)
diff --git a/mlcustomize/guest_packages.ml b/mlcustomize/guest_packages.ml
new file mode 100644
index 000000000000..4c3c34ed5fcd
--- /dev/null
+++ b/mlcustomize/guest_packages.ml
@@ -0,0 +1,132 @@
+(* virt-customize
+ * Copyright (C) 2012-2021 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *)
+
+open Printf
+
+open Common_gettext.Gettext
+open Std_utils
+
+exception Unknown_package_manager of string
+exception Unimplemented_package_manager of string
+
+(* Windows has package_management == "unknown". *)
+let error_unknown_package_manager flag +  let msg = sprintf (f_"cannot use
?%s? because no package manager has been \
+                        detected for this guest OS.\n\nIf this guest OS is a \
+                        common one with ordinary package management then this \
+                        may have been caused by a failure of libguestfs \
+                        inspection.\n\nFor OSes such as Windows that lack \
+                        package management, this is not possible.  Try using \
+                        one of the ?--firstboot*? flags instead (described in \
+                        the virt-customize(1) manual).") flag in
+  raise (Unknown_package_manager msg)
+
+let error_unimplemented_package_manager flag pm +  let msg = sprintf
(f_"sorry, ?%s? with the ?%s? package manager has not \
+                        been implemented yet.\n\nYou can work around this by \
+                        using one of the ?--run*? or ?--firstboot*? options \
+                        instead (described in the virt-customize(1)
manual).")
+                    flag pm in
+  raise (Unimplemented_package_manager msg)
+
+(* http://distrowatch.com/dwres.php?resource=package-management *)
+let install_command packages package_management +  let quoted_args =
String.concat " " (List.map quote packages) in
+  match package_management with
+  | "apk" ->
+     sprintf "
+       apk update
+       apk add %s
+     " quoted_args
+  | "apt" ->
+    (* http://unix.stackexchange.com/questions/22820 *)
+    sprintf "
+      export DEBIAN_FRONTEND=noninteractive
+      apt_opts='-q -y -o Dpkg::Options::=--force-confnew'
+      apt-get $apt_opts update
+      apt-get $apt_opts install %s
+    " quoted_args
+  | "dnf" ->
+     sprintf "dnf%s -y install %s"
+             (if verbose () then " --verbose" else "")
+             quoted_args
+  | "pisi" ->   sprintf "pisi it %s" quoted_args
+  | "pacman" -> sprintf "pacman -S --noconfirm %s"
quoted_args
+  | "urpmi" ->  sprintf "urpmi %s" quoted_args
+  | "xbps" ->   sprintf "xbps-install -Sy %s"
quoted_args
+  | "yum" ->    sprintf "yum -y install %s" quoted_args
+  | "zypper" -> sprintf "zypper -n in -l %s" quoted_args
+
+  | "unknown" ->
+    error_unknown_package_manager (s_"--install")
+  | pm ->
+    error_unimplemented_package_manager (s_"--install") pm
+
+let update_command package_management +  match package_management with
+  | "apk" ->
+     "
+       apk update
+       apk upgrade
+     "
+  | "apt" ->
+    (* http://unix.stackexchange.com/questions/22820 *)
+    "
+      export DEBIAN_FRONTEND=noninteractive
+      apt_opts='-q -y -o Dpkg::Options::=--force-confnew'
+      apt-get $apt_opts update
+      apt-get $apt_opts upgrade
+    "
+  | "dnf" ->
+     sprintf "dnf%s -y --best upgrade"
+             (if verbose () then " --verbose" else "")
+  | "pisi" ->   "pisi upgrade"
+  | "pacman" -> "pacman -Su"
+  | "urpmi" ->  "urpmi --auto-select"
+  | "xbps" ->   "xbps-install -Suy"
+  | "yum" ->    "yum -y update"
+  | "zypper" -> "zypper -n update -l"
+
+  | "unknown" ->
+    error_unknown_package_manager (s_"--update")
+  | pm ->
+    error_unimplemented_package_manager (s_"--update") pm
+
+let uninstall_command packages package_management +  let quoted_args =
String.concat " " (List.map quote packages) in
+  match package_management with
+  | "apk" -> sprintf "apk del %s" quoted_args
+  | "apt" ->
+    (* http://unix.stackexchange.com/questions/22820 *)
+    sprintf "
+      export DEBIAN_FRONTEND=noninteractive
+      apt_opts='-q -y -o Dpkg::Options::=--force-confnew'
+      apt-get $apt_opts remove %s
+    " quoted_args
+  | "dnf" ->    sprintf "dnf -y remove %s" quoted_args
+  | "pisi" ->   sprintf "pisi rm %s" quoted_args
+  | "pacman" -> sprintf "pacman -R %s" quoted_args
+  | "urpmi" ->  sprintf "urpme %s" quoted_args
+  | "xbps" ->   sprintf "xbps-remove -Sy %s" quoted_args
+  | "yum" ->    sprintf "yum -y remove %s" quoted_args
+  | "zypper" -> sprintf "zypper -n rm %s" quoted_args
+
+  | "unknown" ->
+    error_unknown_package_manager (s_"--uninstall")
+  | pm ->
+    error_unimplemented_package_manager (s_"--uninstall") pm
-- 
2.19.1.3.g30247aa5d201

Replace the "guest_install_command", "guest_update_command"
and
"guest_uninstall_command" helper functions with the corresponding
functions from libguestfs-common, interface mlcustomize/Guest_packages.

Add a wrapper function for (a) dealing with the exceptions uniformly
(keeping the original behavior of virt-customize), (b) centralizing the
[g#inspect_get_package_management root] call. Regarding (b), the wrapper
function fills in the last argument [package_management] of the
Guest_packages functions; thus, pass partially applied functions to the
wrapper at the original call sites.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    Regression-tested with:
    
    virt-builder fedora-35
    virt-customize -a fedora-35.img -v -x --no-logfile --install
qemu-guest-agent
    virt-customize -a fedora-35.img -v -x --no-logfile --uninstall
qemu-guest-agent
    virt-customize -a fedora-35.img -v -x --no-logfile --update

 customize/customize_run.ml | 106 ++------------------
 common                     |   2 +-
 2 files changed, 10 insertions(+), 98 deletions(-)

diff --git a/customize/customize_run.ml b/customize/customize_run.ml
index 99b5fe14d849..bb2ba2a03221 100644
--- a/customize/customize_run.ml
+++ b/customize/customize_run.ml
@@ -67,99 +67,11 @@ let run (g : G.guestfs) root (ops : ops)          error
(f_"%s: command exited with an error") display
   in
 
-  (* http://distrowatch.com/dwres.php?resource=package-management *)
-  let rec guest_install_command packages -    let quoted_args = String.concat
" " (List.map quote packages) in
-    match g#inspect_get_package_management root with
-    | "apk" ->
-       sprintf "
-         apk update
-         apk add %s
-       " quoted_args
-    | "apt" ->
-      (* http://unix.stackexchange.com/questions/22820 *)
-      sprintf "
-        export DEBIAN_FRONTEND=noninteractive
-        apt_opts='-q -y -o Dpkg::Options::=--force-confnew'
-        apt-get $apt_opts update
-        apt-get $apt_opts install %s
-      " quoted_args
-    | "dnf" ->
-       sprintf "dnf%s -y install %s"
-               (if verbose () then " --verbose" else "")
-               quoted_args
-    | "pisi" ->   sprintf "pisi it %s" quoted_args
-    | "pacman" -> sprintf "pacman -S --noconfirm %s"
quoted_args
-    | "urpmi" ->  sprintf "urpmi %s" quoted_args
-    | "xbps" ->   sprintf "xbps-install -Sy %s"
quoted_args
-    | "yum" ->    sprintf "yum -y install %s"
quoted_args
-    | "zypper" -> sprintf "zypper -n in -l %s"
quoted_args
-
-    | "unknown" ->
-      error_unknown_package_manager (s_"--install")
-    | pm ->
-      error_unimplemented_package_manager (s_"--install") pm
-
-  and guest_update_command () -    match g#inspect_get_package_management root
with
-    | "apk" ->
-       "
-         apk update
-         apk upgrade
-       "
-    | "apt" ->
-      (* http://unix.stackexchange.com/questions/22820 *)
-      "
-        export DEBIAN_FRONTEND=noninteractive
-        apt_opts='-q -y -o Dpkg::Options::=--force-confnew'
-        apt-get $apt_opts update
-        apt-get $apt_opts upgrade
-      "
-    | "dnf" ->
-       sprintf "dnf%s -y --best upgrade"
-               (if verbose () then " --verbose" else "")
-    | "pisi" ->   "pisi upgrade"
-    | "pacman" -> "pacman -Su"
-    | "urpmi" ->  "urpmi --auto-select"
-    | "xbps" ->   "xbps-install -Suy"
-    | "yum" ->    "yum -y update"
-    | "zypper" -> "zypper -n update -l"
-
-    | "unknown" ->
-      error_unknown_package_manager (s_"--update")
-    | pm ->
-      error_unimplemented_package_manager (s_"--update") pm
-
-  and guest_uninstall_command packages -    let quoted_args = String.concat
" " (List.map quote packages) in
-    match g#inspect_get_package_management root with
-    | "apk" -> sprintf "apk del %s" quoted_args
-    | "apt" ->
-      (* http://unix.stackexchange.com/questions/22820 *)
-      sprintf "
-        export DEBIAN_FRONTEND=noninteractive
-        apt_opts='-q -y -o Dpkg::Options::=--force-confnew'
-        apt-get $apt_opts remove %s
-      " quoted_args
-    | "dnf" ->    sprintf "dnf -y remove %s" quoted_args
-    | "pisi" ->   sprintf "pisi rm %s" quoted_args
-    | "pacman" -> sprintf "pacman -R %s" quoted_args
-    | "urpmi" ->  sprintf "urpme %s" quoted_args
-    | "xbps" ->   sprintf "xbps-remove -Sy %s"
quoted_args
-    | "yum" ->    sprintf "yum -y remove %s" quoted_args
-    | "zypper" -> sprintf "zypper -n rm %s" quoted_args
-
-    | "unknown" ->
-      error_unknown_package_manager (s_"--uninstall")
-    | pm ->
-      error_unimplemented_package_manager (s_"--uninstall") pm
-
-  (* Windows has package_management == "unknown". *)
-  and error_unknown_package_manager flag -    error (f_"cannot use ?%s?
because no package manager has been detected for this guest OS.\n\nIf this guest
OS is a common one with ordinary package management then this may have been
caused by a failure of libguestfs inspection.\n\nFor OSes such as Windows that
lack package management, this is not possible.  Try using one of the
?--firstboot*? flags instead (described in the manual).") flag
-
-  and error_unimplemented_package_manager flag pm -      error (f_"sorry,
?%s? with the ?%s? package manager has not been implemented yet.\n\nYou can work
around this by using one of the ?--run*? or ?--firstboot*? options instead
(described in the manual).") flag pm
+  let guest_pkgs_command f +    try f (g#inspect_get_package_management root)
with
+    | Guest_packages.Unknown_package_manager msg
+    | Guest_packages.Unimplemented_package_manager msg ->
+      error "%s" msg
   in
 
   (* Set the random seed. *)
@@ -255,7 +167,7 @@ let run (g : G.guestfs) root (ops : ops)      |
`FirstbootPackages pkgs ->
       message (f_"Installing firstboot packages: %s")
         (String.concat " " pkgs);
-      let cmd = guest_install_command pkgs in
+      let cmd = guest_pkgs_command (Guest_packages.install_command pkgs) in
       let name = String.concat " " ("install" :: pkgs) in
       Firstboot.add_firstboot_script g root name cmd
 
@@ -271,7 +183,7 @@ let run (g : G.guestfs) root (ops : ops)  
     | `InstallPackages pkgs ->
       message (f_"Installing packages: %s") (String.concat "
" pkgs);
-      let cmd = guest_install_command pkgs in
+      let cmd = guest_pkgs_command (Guest_packages.install_command pkgs) in
       do_run ~display:cmd ~warn_failed_no_network:true cmd
 
     | `Link (target, links) ->
@@ -365,12 +277,12 @@ let run (g : G.guestfs) root (ops : ops)  
     | `UninstallPackages pkgs ->
       message (f_"Uninstalling packages: %s") (String.concat "
" pkgs);
-      let cmd = guest_uninstall_command pkgs in
+      let cmd = guest_pkgs_command (Guest_packages.uninstall_command pkgs) in
       do_run ~display:cmd cmd
 
     | `Update ->
       message (f_"Updating packages");
-      let cmd = guest_update_command () in
+      let cmd = guest_pkgs_command Guest_packages.update_command in
       do_run ~display:cmd ~warn_failed_no_network:true cmd
 
     | `Upload (path, dest) ->
diff --git a/common b/common
index f8de5508fe75..79828d1a92a4 160000
--- a/common
+++ b/common
@@ -1 +1 @@
-Subproject commit f8de5508fe755acca99c9ab40c501d6c3e2bcb1e
+Subproject commit 79828d1a92a407edd8f946db5cc1bf07d260bf2f
-- 
2.19.1.3.g30247aa5d201

On Mon, Jun 06, 2022 at 04:19:37PM +0200, Laszlo Ersek
wrote:
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764
> 
> I'm going to post the pre-requisite libguestfs-common and guestfs-tools
> patches (one patch for each project) in response to this cover letter,
> too.
> 
> I'm not sure why we want to perform the installation specifically at
> *firstboot* <https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c2>.
> Virt-v2v already only supports conversions where host and guest arches
> are identical; thus, the appliance kernel could run native guest
> binaries (if necessary). I've read
> <https://libguestfs.org/guestfs.3.html#running-commands>, but I'm
not
> overly convinced. Firstboot comes with *lots* of complications. Even
> <https://libguestfs.org/virt-builder.1.html#installing-packages>
> mentions some of them:
> 
> > The downsides are that it will take the guest a lot longer to boot
> > first time, and there?s nothing much you can do if package
> > installation fails (eg. if a network problem means the guest can't
> > reach the package repositories).
It is true that for Linux guests, and because virt-v2v assumes host
arch == guest arch, it is usually possible to run commands in the
guest at conversion time.  This is why we can run (for example)
dracut/mkinitrd to regenerate the initramfs.  Firstboot is mainly
useful for Windows where we cannot easily run Windows binaries at
conversion time.

It's possible there is still an issue with conversions where the host
kernel is older or newer than the guest kernel (especially older host)
and so might not be able to run the qemu-ga installation commands.
We've not seen that with dracut so far.

SELinux is mentioned in the bug, but should not be a concern.
(Virt-v2v runs commands in an appliance that has no SELinux, and we
relabel the guest filesystem after that.)

So firstboot shouldn't be necessary from a technical point of view.

OTOH ... my comment here:

https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c2

was about more social issues where we've not been able to put the
qemu-ga RPMs on the ISO, and firstboot seemed like the easiest way to
get around that.  AFAIK there is no way for a host subscribed to (eg)
RHEL 9 channels to download RHEL 7/8 RPMs.  Maybe even hard to grab
SUSE or Debian packages.
> Anyway, here goes.
I'll go ahead and review it as it is now!

Rich.
> Thanks,
> Laszlo
> 
> Laszlo Ersek (4):
>   output/create_libvirt_xml: wire up the QEMU guest agent
>   windows_virtio: remove "install_linux_tools"
>   convert_linux: extract qemu-guest-agent package name
>   convert_linux: install the QEMU guest agent with a firstboot script
> 
>  common                       |   2 +-
>  convert/convert_linux.ml     | 102 ++++++++++++++++++--
>  convert/linux.ml             |  35 -------
>  convert/linux.mli            |  11 ---
>  convert/windows_virtio.ml    |  42 --------
>  convert/windows_virtio.mli   |   4 -
>  output/create_libvirt_xml.ml |  11 +++
>  tests/test-v2v-i-ova.xml     |   4 +
>  8 files changed, 111 insertions(+), 100 deletions(-)
> 
> -- 
> 2.19.1.3.g30247aa5d201
> 
> _______________________________________________
> 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
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org