Libguestfs - Jan 2023 - [p2v PATCH 1/4] ssh: shield virt-v2v from bash RC files

In preparation for some GUI investigation / work, I've run "make
run-virt-p2v-in-a-vm" for the first time on my new RHEL-9.1 laptop. Some
tweaks appear to be necessary (or at least useful).

Laszlo

Laszlo Ersek (4):
  ssh: shield virt-v2v from bash RC files
  make-disk: restrict kernel update bug work-around
  make-disk: replace here-docs with printfs for better indentation
  make-disk: inject EPEL9 repo in RHEL9 disk image

 ssh.c                 |  2 +-
 test-virt-p2v-ssh.sh  |  2 +-
 virt-p2v-make-disk.in | 57 +++++++++++++++++++++++++++++++------------
 3 files changed, 44 insertions(+), 17 deletions(-)

I don't have virt-v2v installed system-wide on my RHEL9 laptop, I rely on
the various "./run" scripts to put everything that virt-p2v's
"make check"
requires on the PATH environment variable.

However, "test-virt-p2v-nbdkit.sh" still breaks for me; it complains
that
"virt-v2v --version" cannot be executed by test_connection(), due to
"virt-v2v" not being found.  Prefixing the "virt-v2v
--version" invocation
in test_connection() with "echo \"$PATH\"; ", I've
determined from
"test-virt-p2v-nbdkit.sh.log" that the *non-appending* PATH=...
variable
assignment from my $HOME/.bashrc takes effect.  It wipes out the PATH
changes from the "./run" scripts, hiding virt-v2v.

I've added the following snippet to my $HOME/.bashrc file, for debugging:
> XXX=mess-$(date --rfc-3339=ns)--$$.log
> /bin/pstree -a -A -l -n -p $$ >| "$HOME/tmp/$XXX"
so that whichever shell read the RC file create a log file, named with a
nanosecond-resolution timestamp and the shell's PID, and record the
"path"
in the process tree that lead to the shell.

The snippet created the following two files:
> mess-2023-01-18 09:33:49.896065330+01:00--36312.log
> mess-2023-01-18 09:33:49.937365639+01:00--36312.log
containing, respectively:
> bash,36312
>   `-pstree,36315 -a -A -l -n -p 36312
and
> bash,36312 --noediting --noprofile
>   `-pstree,36320 -a -A -l -n -p 36312
Note that the PID of the shell is unchanged, but the pstree PID changes.
This means that the same PID (same process) reads the bash RC file twice
-- which can only be explained by the *image* of the process being
replaced, from bash, to bash.

So the problem happens in two places:

- First, when we "exec" the interactive shell in
"test-virt-p2v-ssh.sh",
  that is, our ssh "shim".  Interactive *non-login* shells read the RC
  file, unless the "--norc" option is passed.

- Second, when we "exec" bash from start_ssh(), on the remote machine.

  This invocation already passes the "--noprofile" option, but that
has no
  effect.  "--noprofile" prevents the shell from reading
  "$HOME/.bash_profile" when the shell is a *login* shell, regardless
of
  whether it is interactive or not.  Because the existent
"--noprofile"
  option does not prevent the symptom, we can determine that the remote
  shell started by start_ssh() is a *non-login* shell, and that it's also
  interactive (otherwise it wouldn't read the RC file).  Thus, we need to
  pass "--norc" here as well.

  (While I believe, based on the above, that "--noprofile" is
superfluous,
  I'd like to avoid any potential regressions here, so I'm keeping
  "--noprofile" too.)

Append "--norc" to both command lines.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 ssh.c                | 2 +-
 test-virt-p2v-ssh.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ssh.c b/ssh.c
index aeb57584cf55..513a20318359 100644
--- a/ssh.c
+++ b/ssh.c
@@ -469,7 +469,7 @@ start_ssh (unsigned spawn_flags, struct config *config,
    * We don't know how command line editing is set up
    * (https://bugzilla.redhat.com/1314244#c9).
    */
-  if (mexp_printf (h, "exec bash --noediting --noprofile\n") == -1) {
+  if (mexp_printf (h, "exec bash --noediting --noprofile --norc\n")
== -1) {
     set_ssh_mexp_error ("mexp_printf");
     mexp_close (h);
     return NULL;
diff --git a/test-virt-p2v-ssh.sh b/test-virt-p2v-ssh.sh
index 8a14b71fbd4b..f8b86b539ffe 100755
--- a/test-virt-p2v-ssh.sh
+++ b/test-virt-p2v-ssh.sh
@@ -57,4 +57,4 @@ while true ; do
 done
 
 # Now run the interactive shell.
-exec bash
+exec bash --norc

Commit 2764abefaa14 ("make-disk: Don't upgrade the kernel to avoid
kernel
command line bug", 2022-03-24) replaced the "--update" option of
virt-builder with an open-coded DNF update command, excluding the kernel
packages from the update.

Albeit a justified workaround, it is a bit too broad: it breaks on all
guest OS images that don't have DNF, such as old RHEL, Debian and Ubuntu,
etc.  (In a subsequent patch, I'm going to refactor the Debian/Ubuntu
branch, and for testing that, I first need eliminate the open-coded DNF
command.)  Refine the workaround as follows:

- I've tested Fedora 33 and Fedora 37; they work without the workaround
  (they are not yet, and no longer, affected by the bug, respectively).

- I've tested Fedora 34 and 36, they are affected.  They need the
  workaround.

- Fedora 35 is also broken, per
  <https://bugzilla.redhat.com/show_bug.cgi?id=1945835#c24>.

- RHEL-9.0 and RHEL-9.1 *seem* to work fine without the workaround (that
  is, with commit 2764abefaa14 reverted).  However, that's deceptive: in
  my testing, the original "--update" option does not bring in any new
  kernel packages, so even if the bug is still there, it is not triggered.
  Because RHEL-9 is based on Fedora 34, which is affected, assume that
  RHEL-9 is affected too.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 virt-p2v-make-disk.in | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/virt-p2v-make-disk.in b/virt-p2v-make-disk.in
index 865a92847f8b..44ff0b45ac7a 100644
--- a/virt-p2v-make-disk.in
+++ b/virt-p2v-make-disk.in
@@ -48,6 +48,7 @@ eval set -- "$TEMP"
 output upload verbose+declare -a update_option=(--update)
 declare -a passthru
 
 usage ()
@@ -184,6 +185,19 @@ EOF
           --upload $tmpdir/p2v.conf:/etc/dracut.conf.d/
           --run $tmpdir/post-install
         "
+        # Fedora 34 through 36, and presumably RHEL-9 (based on Fedora 34)
+        # suffer from a bug where a kernel image installation via libguestfs
+        # pollutes the just installed kernel's commandline in the boot
loader
+        # config file.  Irrelevant and bogus parameters from the appliance
+        # kernel's /proc/cmdline are copied over.  This prevents the root
+        # filesystem from being mounted.  Refer to
+        # <https://bugzilla.redhat.com/show_bug.cgi?id=1945835#c24>
and/or
+        # commit 2764abefaa14.
+        case "$osversion" in
+            fedora-34|fedora-35|fedora-36|rhel-9.*)
+                update_option=(--run-command 'dnf -y update
--exclude=kernel\*')
+                ;;
+        esac
         ;;
     debian-*|ubuntu-*)
         depsfile="$datadir/dependencies.debian"
@@ -246,7 +260,7 @@ virt-builder "$osversion"                         
\
     $preinstall_args                                            \
     --hostname p2v.local                                        \
     --run-command 'hostname p2v.local'                          \
-    --run-command 'dnf -y update --exclude=kernel\*'            \
+    "${update_option[@]}"                                       \
     --install "$install"                                        \
     --root-password password:p2v                                \
     --upload "$datadir"/issue:/etc/issue                        \

Here-documents break the indentation of the script; replace them with the
"printf" shell utility. (We could use the <<- redirection
operator for
stripping leading TABs from the here-docs, but the script itself does not
use TABs for indentation, so that would introduce a different kind of
inconsistency.)

Because we use a quoted word ('EOF') for our delimiter in the
here-documents, variables in the here-docs are not expanded; stick with
that.

Use a sole '%s\n' format operand with each printf invocation:
"[t]he
/format/ operand shall be reused as often as necessary to satisfy the
/argument/ operands"
<https://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html>.

(I've tried to regression-test this with the two latest Ubuntu images:
ubuntu-18.04, ubuntu-20.04. The former wouldn't build, and the latter only
booted to a GDM login screen. My build host is RHEL-9.1, so the kiosk is
not actually expected to work. Any further investigation here is left to
Ubuntu users.

The Fedora 37 image works fine, even with the virt-p2v binary coming from
RHEL-9.1.)

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 virt-p2v-make-disk.in | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/virt-p2v-make-disk.in b/virt-p2v-make-disk.in
index 44ff0b45ac7a..0bf7f0e0414f 100644
--- a/virt-p2v-make-disk.in
+++ b/virt-p2v-make-disk.in
@@ -170,15 +170,15 @@ xzcat "$virt_p2v_xz_binary" >
"$virt_p2v_binary"
 case "$osversion" in
     centos-*|fedora-*|rhel-*|scientificlinux-*|oraclelinux-*)
         depsfile="$datadir/dependencies.redhat"
-        cat > $tmpdir/p2v.conf <<'EOF'
-add_drivers+=" usb-storage "
-EOF
-        cat > $tmpdir/post-install <<'EOF'
-#!/bin/bash
-# Rebuild the initramfs.
-latest_version="$(cd /lib/modules; ls -1vr | head -1)"
-dracut -v -f --kver $latest_version
-EOF
+        printf '%s\n'                      \
+            'add_drivers+=" usb-storage "' \
+            > $tmpdir/p2v.conf
+        printf '%s\n'                                                \
+            '#!/bin/bash'                                            \
+            '# Rebuild the initramfs.'                               \
+            'latest_version="$(cd /lib/modules; ls -1vr | head
-1)"' \
+            'dracut -v -f --kver $latest_version'                    \
+            > $tmpdir/post-install
         # Double quotes because we want $tmpdir to be expanded:
         extra_args="
           --selinux-relabel
@@ -201,11 +201,11 @@ EOF
         ;;
     debian-*|ubuntu-*)
         depsfile="$datadir/dependencies.debian"
-        cat > $tmpdir/policy-rc.d <<'EOF'
-#!/bin/sh
-# Avoid running daemons during the upgrade/installation
-exit 101
-EOF
+        printf '%s\n' \                                               \
+            '#!/bin/sh'                                               \
+            '# Avoid running daemons during the upgrade/installation' \
+            'exit 101'                                                \
+            > $tmpdir/policy-rc.d
         chmod +x $tmpdir/policy-rc.d
         # Double quotes because we want $tmpdir to be expanded:
         preinstall_args="

In RHEL9, "metacity" was replaced with "gnome-kiosk":

  https://bugzilla.redhat.com/show_bug.cgi?id=1951145#c4
  https://bugzilla.redhat.com/show_bug.cgi?id=1956984#c1

gnome-kiosk has particular requirements for the main application:

  https://bugzilla.redhat.com/show_bug.cgi?id=1999060#c13

We don't know if that's compatible with our separately opened XTerm
window(s). For now, let's stick with metacity, by pulling in EPEL9. We can
work on RHEL9 purity (including gnome-kiosk fixes and/or compatibility)
once a "standard" RHEL9-based image builder service becomes a
realistic
prospect.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 virt-p2v-make-disk.in | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/virt-p2v-make-disk.in b/virt-p2v-make-disk.in
index 0bf7f0e0414f..63001debbf3e 100644
--- a/virt-p2v-make-disk.in
+++ b/virt-p2v-make-disk.in
@@ -48,6 +48,7 @@ eval set -- "$TEMP"
 output upload verbose+declare -a install_repo_packages_option
 declare -a update_option=(--update)
 declare -a passthru
 
@@ -198,6 +199,17 @@ case "$osversion" in
                 update_option=(--run-command 'dnf -y update
--exclude=kernel\*')
                 ;;
         esac
+        # For RHEL-9, we need to grab the metacity window manager from EPEL-9.
+        # Enable the EPEL-9 repository by installing the RPM containing the
+        # *.repo file(s) directly from the network.
+        case "$osversion" in
+            rhel-9.*)
+                install_repo_packages_option=(
+                  --install
+                 
https://dl.fedoraproject.org/pub/epel/epel-release-latest-9.noarch.rpm
+                )
+                ;;
+        esac
         ;;
     debian-*|ubuntu-*)
         depsfile="$datadir/dependencies.debian"
@@ -260,6 +272,7 @@ virt-builder "$osversion"                         
\
     $preinstall_args                                            \
     --hostname p2v.local                                        \
     --run-command 'hostname p2v.local'                          \
+    "${install_repo_packages_option[@]}"                        \
     "${update_option[@]}"                                       \
     --install "$install"                                        \
     --root-password password:p2v                                \