[Libguestfs] [v2v PATCH] convert_linux: start the QEMU guest agent in a distro-specific way

[Richard W.M. Jones]

On Wed, Aug 17, 2022 at 04:47:36PM +0200, Laszlo Ersek wrote:
> The current command "service <package-name> start" does not apply to
> RHEL-6; the service name ("qemu-ga") differs from the package name
> ("qemu-guest-agent") there.
> 
> Overhaul the logic -- detach the command from the package name; cover the
> RHEL, ALT, SUSE and Debian families separately. Remove the "chkconfig"
> command, as in all tested / investigated cases, it is unnecessary.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>  convert/convert_linux.ml | 56 ++++++++++++++++++++++++++++------------
>  1 file changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
> index 2aaa438e42ac..b8e9ad15e22d 100644
> --- a/convert/convert_linux.ml
> +++ b/convert/convert_linux.ml
> @@ -66,6 +66,34 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ =
>      | _ -> None
>    in
>  
> +  let qga_svc_start_cmd family distro major =
> +    match family, distro, major with
> +    | `RHEL_family, ( "rhel" | "centos" | "scientificlinux" | "redhat-based" |
> +                      "oraclelinux" ), 6 ->

Shouldn't this be:
                                 ...    , i when i <= 6 ->

?

> +      (* https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c52 *)
> +      Some "service qemu-ga start"
> +
> +    | `RHEL_family, _, _ ->
> +      (* https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c52 *)
> +      Some "systemctl start qemu-guest-agent"
> +
> +    | `ALT_family, _, _ ->
> +      (* https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c45 *)
> +      Some "systemctl start qemu-guest-agent"
> +
> +    | `SUSE_family, _, _ ->
> +      (* https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c51 *)
> +      None
> +
> +    | `Debian_family, _, _ ->
> +      (* https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c42 *)
> +      Some "service qemu-guest-agent start"
> +
> +    | _ ->
> +      (* should never be called when "qga_pkg_of_family" returns None *)
> +      assert false
> +  in
> +
>    assert (inspect.i_package_format = "rpm" || inspect.i_package_format = "deb");
>  
>    (* Fail early if i_apps is empty.  Certain steps such as kernel
> @@ -615,23 +643,19 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ =
>                          \ \ 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.
> +            (* On all the distro families covered by "qga_pkg_of_family" and
> +             * "qga_svc_start_cmd", the QEMU guest agent service is always
> +             * enabled by package installation for *subsequent* boots. Package
> +             * installation may or may not enable the service for the current
> +             * (i.e., first) boot, however, so try that here manually.
>               *)
> -            fbs "start qga"
> -              (sprintf "#!/bin/sh\n\
> -                        service %s start\n\
> -                        chkconfig %s on\n" qga_pkg qga_pkg)
> +            match qga_svc_start_cmd family inspect.i_distro inspect.i_major_version
> +            with
> +            | None -> ()
> +            | Some start_cmd ->
> +              fbs "start qga"
> +                (sprintf "#!/bin/sh\n\
> +                          %s\n" start_cmd)
>            with
>            | Guest_packages.Unknown_package_manager msg
>            | Guest_packages.Unimplemented_package_manager msg ->

If you wanted to reduce the long range dependency between
qga_pkg_of_family and qga_svc_start_cmd (and hence the assert) you
could make a qga_something function that returns both the package name
and the service start command as a pair of strings (or None).  It
could make the code more maintainable long term.

It all looks fine, so ACK either way.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top