Libguestfs - Oct 2016 - Re: [PATCH 2/3] v2v: linux: check also kernel config for modules

Hi,

this series adds a couple bits more in v2v to convert Debian/Ubuntu
(and derived) guests.  The series does not complete the support (see
known issues below), but all the patches here should be fit for review
and inclusion.  The series does not enable the conversion, yet.

Known issues:

* currently tested with simple local guest images, hence needs testing
  with real guests on libvirt/VMware/Xen/VirtualBox/etc (which have
  various tools to be removed/tweaked)

* is tweaking the "enterprise-linux" module the correct/wanted way?
  should it be renamed to just "linux" then?

* surely something else I'm missing

Thanks,


Pino Toscano (3):
  v2v: linux kernels: set type of 'g'
  v2v: linux: check also kernel config for modules
  v2v: bootloaders: improve detection of Grub2 default method

 v2v/linux_bootloaders.ml | 61 ++++++++++++++++++++++++++++++++++--------------
 v2v/linux_kernels.ml     | 22 +++++++++++++++--
 2 files changed, 64 insertions(+), 19 deletions(-)

-- 
2.7.4

Make sure the type checker can detect the real type of 'g', even when
adding more code using it in detect_kernels.
---
 v2v/linux_kernels.ml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/v2v/linux_kernels.ml b/v2v/linux_kernels.ml
index 3819d26..b292921 100644
--- a/v2v/linux_kernels.ml
+++ b/v2v/linux_kernels.ml
@@ -49,7 +49,7 @@ let string_of_kernel_info ki      (match ki.ki_initrd with
None -> "None" | Some f -> f)
     ki.ki_supports_virtio ki.ki_is_xen_kernel ki.ki_is_debug
 
-let detect_kernels g inspect family bootloader +let detect_kernels (g :
G.guestfs) inspect family bootloader    (* What kernel/kernel-like packages are
installed on the current guest? *)
   let installed_kernels : kernel_info list      let rex_ko = Str.regexp
".*\\.k?o\\(\\.xz\\)?$" in
-- 
2.7.4

When checking whether a kernel supports virtio or it is Xen-based, it is
assumed that the feature has the kernel module for it; this will fail if
the feature is built-in in the kernel, misrepresenting it.

The solution is to check the kernel configuration (/boot/config-$kver)
whether the feature is built-in, in case there is no module available.

This fixes the virtio detection on Ubuntu kernels, where virtio is
built in and not as module.
---
 v2v/linux_kernels.ml | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/v2v/linux_kernels.ml b/v2v/linux_kernels.ml
index b292921..c197f78 100644
--- a/v2v/linux_kernels.ml
+++ b/v2v/linux_kernels.ml
@@ -53,6 +53,21 @@ let detect_kernels (g : G.guestfs) inspect family bootloader 
(* What kernel/kernel-like packages are installed on the current guest? *)
   let installed_kernels : kernel_info list      let rex_ko = Str.regexp
".*\\.k?o\\(\\.xz\\)?$" in
+    let check_config version feature +      let prefix = "^CONFIG_" ^
String.uppercase_ascii feature ^ "=" in
+      let lines = g#grep ~extended:true prefix ("/boot/config-" ^
version) in
+      let lines = Array.to_list lines in
+      match lines with
+      | [] -> false
+      | line :: _ ->
+        let kind = snd (String.split "=" line) in
+        (match kind with
+        | "m" (* Theoretically this should not be needed, since the
module
+               * would be found. *)
+        | "y" -> true
+        | _ -> false
+        )
+    in
     let rex_ko_extract = Str.regexp
".*/\\([^/]+\\)\\.k?o\\(\\.xz\\)?$" in
     let rex_initrd        if family = `Debian_family then
@@ -156,7 +171,10 @@ let detect_kernels (g : G.guestfs) inspect family
bootloader               ) modules in
              assert (List.length modules > 0);
 
-             let supports_virtio = List.mem "virtio_net" modules in
+             let kernel_supports what kconf +               List.mem what
modules || check_config version kconf in
+
+             let supports_virtio = kernel_supports "virtio_net"
"VIRTIO_NET" in
              let is_xen_kernel = List.mem "xennet" modules in
 
              (* If the package name is like "kernel-debug", then
it's
-- 
2.7.4

Detect only once which method must be used to get and set the default
Grub2 kernel in the guest: this avoids the same checks in list_kernels
and set_default_kernel.

Also, add a "no method" option as well: Debian/Ubuntu guests do not
have
neither grubby nor Perl's Bootloader::Tools, so there is no way to know
what is the default kernel, nor to change it.  In this case, add a
warning about this situation.
---
 v2v/linux_bootloaders.ml | 61 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 17 deletions(-)

diff --git a/v2v/linux_bootloaders.ml b/v2v/linux_bootloaders.ml
index 4f53f87..e03d22b 100644
--- a/v2v/linux_bootloaders.ml
+++ b/v2v/linux_bootloaders.ml
@@ -184,6 +184,12 @@ object
     g#aug_save ()
 end
 
+(** The method used to get and set the default kernel in Grub2. *)
+type default_kernel_method +  | MethodGrubby  (** Use the 'grubby'
tool. *)
+  | MethodPerlBootloader  (** Use the 'Bootloader::Tools' Perl module.
*)
+  | MethodNone  (** No known way. *)
+
 (* Grub2 representation. *)
 class bootloader_grub2 (g : G.guestfs) grub_config  
@@ -199,6 +205,20 @@ class bootloader_grub2 (g : G.guestfs) grub_config       
error (f_"failed to find grub2-mkconfig binary (but Grub2 was detected on
guest)")
   in
 
+  let get_default_method +    let has_perl_bootloader () +      try
+        ignore (g#command [| "/usr/bin/perl";
"-MBootloader::Tools"; "-e1" |]);
+        true
+      with G.Error _ -> false
+    in
+    if g#exists "/sbin/grubby" then MethodGrubby
+    else if has_perl_bootloader () then MethodPerlBootloader
+    else (
+      warning (f_"could not determine a way to update the configuration of
Grub2");
+      MethodNone
+    ) in
+
 object (self)
   inherit bootloader
 
@@ -250,18 +270,23 @@ object (self)
 
   method list_kernels      let get_default_image () -      let cmd -        if
g#exists "/sbin/grubby" then
-          [| "grubby"; "--default-kernel" |]
-        else
-          [| "/usr/bin/perl"; "-MBootloader::Tools";
"-e"; "
-                InitLibrary();
-                my $default = Bootloader::Tools::GetDefaultSection();
-                print $default->{image};
-             " |] in
-      match g#command cmd with
-      | "" -> None
-      | k ->
+      let res +        match get_default_method with
+        | MethodGrubby ->
+          Some (g#command [| "grubby"; "--default-kernel"
|])
+        | MethodPerlBootloader ->
+          let cmd +            [| "/usr/bin/perl";
"-MBootloader::Tools"; "-e"; "
+                  InitLibrary();
+                  my $default = Bootloader::Tools::GetDefaultSection();
+                  print $default->{image};
+               " |] in
+          Some (g#command cmd)
+        | MethodNone ->
+          None in
+      match res with
+      | None -> None
+      | Some k ->
         let len = String.length k in
         let k            if len > 0 && k.[len-1] = '\n' then
@@ -285,10 +310,11 @@ object (self)
     vmlinuzes
 
   method set_default_kernel vmlinuz -    let cmd -      if g#exists
"/sbin/grubby" then
-        [| "grubby"; "--set-default"; vmlinuz |]
-      else
+    match get_default_method with
+    | MethodGrubby ->
+      ignore (g#command [| "grubby"; "--set-default";
vmlinuz |])
+    | MethodPerlBootloader ->
+      let cmd          [| "/usr/bin/perl";
"-MBootloader::Tools"; "-e"; sprintf "
             InitLibrary();
             my @sections = GetSectionList(type=>image,
image=>\"%s\");
@@ -296,7 +322,8 @@ object (self)
             my $newdefault = $section->{name};
             SetGlobals(default, \"$newdefault\");
           " vmlinuz |] in
-    ignore (g#command cmd)
+      ignore (g#command cmd)
+    | MethodNone -> ()
 
   method configure_console = self#grub2_update_console ~remove:false
 
-- 
2.7.4

On Tue, 27 Sep 2016 17:58:12 +0200
Pino Toscano <ptoscano@redhat.com> wrote:
> Detect only once which method must be used to get and set the default
> Grub2 kernel in the guest: this avoids the same checks in list_kernels
> and set_default_kernel.
> 
> Also, add a "no method" option as well: Debian/Ubuntu guests do
not have
> neither grubby nor Perl's Bootloader::Tools, so there is no way to know
> what is the default kernel, nor to change it.  In this case, add a
> warning about this situation.
> ---
>  v2v/linux_bootloaders.ml | 61
++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 17 deletions(-)
> 
The series LGTM.

-- 
Tomáš Golembiovský <tgolembi@redhat.com>

On Tue, Sep 27, 2016 at 05:58:10PM +0200, Pino Toscano
wrote:
> Make sure the type checker can detect the real type of 'g', even
when
> adding more code using it in detect_kernels.
> ---
>  v2v/linux_kernels.ml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/v2v/linux_kernels.ml b/v2v/linux_kernels.ml
> index 3819d26..b292921 100644
> --- a/v2v/linux_kernels.ml
> +++ b/v2v/linux_kernels.ml
> @@ -49,7 +49,7 @@ let string_of_kernel_info ki >      (match
ki.ki_initrd with None -> "None" | Some f -> f)
>      ki.ki_supports_virtio ki.ki_is_xen_kernel ki.ki_is_debug
>  
> -let detect_kernels g inspect family bootloader > +let detect_kernels (g
: G.guestfs) inspect family bootloader >    (* What kernel/kernel-like
packages are installed on the current guest? *)
>    let installed_kernels : kernel_info list >      let rex_ko =
Str.regexp ".*\\.k?o\\(\\.xz\\)?$" in
Trivial change, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Tue, Sep 27, 2016 at 05:58:11PM +0200, Pino Toscano
wrote:
> When checking whether a kernel supports virtio or it is Xen-based, it is
> assumed that the feature has the kernel module for it; this will fail if
> the feature is built-in in the kernel, misrepresenting it.
> 
> The solution is to check the kernel configuration (/boot/config-$kver)
> whether the feature is built-in, in case there is no module available.
> 
> This fixes the virtio detection on Ubuntu kernels, where virtio is
> built in and not as module.
> ---
>  v2v/linux_kernels.ml | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/v2v/linux_kernels.ml b/v2v/linux_kernels.ml
> index b292921..c197f78 100644
> --- a/v2v/linux_kernels.ml
> +++ b/v2v/linux_kernels.ml
> @@ -53,6 +53,21 @@ let detect_kernels (g : G.guestfs) inspect family
bootloader >    (* What kernel/kernel-like packages are installed on the
current guest? *)
>    let installed_kernels : kernel_info list >      let rex_ko =
Str.regexp ".*\\.k?o\\(\\.xz\\)?$" in
> +    let check_config version feature > +      let prefix =
"^CONFIG_" ^ String.uppercase_ascii feature ^ "=" in
> +      let lines = g#grep ~extended:true prefix ("/boot/config-"
^ version) in
> +      let lines = Array.to_list lines in
> +      match lines with
> +      | [] -> false
> +      | line :: _ ->
> +        let kind = snd (String.split "=" line) in
> +        (match kind with
> +        | "m" (* Theoretically this should not be needed, since
the module
> +               * would be found. *)
> +        | "y" -> true
I wouldn't bother with the long comment since it's stating the
obvious, and just do:

  | "m" | "y" -> true
> +        | _ -> false
> +        )
> +    in
>      let rex_ko_extract = Str.regexp
".*/\\([^/]+\\)\\.k?o\\(\\.xz\\)?$" in
>      let rex_initrd >        if family = `Debian_family then
> @@ -156,7 +171,10 @@ let detect_kernels (g : G.guestfs) inspect family
bootloader >               ) modules in
>               assert (List.length modules > 0);
>  
> -             let supports_virtio = List.mem "virtio_net" modules
in
> +             let kernel_supports what kconf > +               List.mem
what modules || check_config version kconf in
> +
> +             let supports_virtio = kernel_supports "virtio_net"
"VIRTIO_NET" in
>               let is_xen_kernel = List.mem "xennet" modules in
>  
>               (* If the package name is like "kernel-debug", then
it's
Yes this is something we should have been doing for a long time.

ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Actually I have another comment about this patch ..

On Tue, Sep 27, 2016 at 05:58:11PM +0200, Pino Toscano
wrote:
> diff --git a/v2v/linux_kernels.ml b/v2v/linux_kernels.ml
> index b292921..c197f78 100644
> --- a/v2v/linux_kernels.ml
> +++ b/v2v/linux_kernels.ml
> @@ -53,6 +53,21 @@ let detect_kernels (g : G.guestfs) inspect family
bootloader >    (* What kernel/kernel-like packages are installed on the
current guest? *)
>    let installed_kernels : kernel_info list >      let rex_ko =
Str.regexp ".*\\.k?o\\(\\.xz\\)?$" in
> +    let check_config version feature > +      let prefix =
"^CONFIG_" ^ String.uppercase_ascii feature ^ "=" in
> +      let lines = g#grep ~extended:true prefix ("/boot/config-"
^ version) in
This could fail if the config file doesn't exist or has an
unexpected name.

I don't think we should fail in that case.

I think what we should do is put the config file name in the
Linux_kernels.kernel_info struct, eg:

  ki_config_file : string option;

where None would be "not found".

The check_config function can then short-circuit when ki_config_file == None.

Rich.
> +      let lines = Array.to_list lines in
> +      match lines with
> +      | [] -> false
> +      | line :: _ ->
> +        let kind = snd (String.split "=" line) in
> +        (match kind with
> +        | "m" (* Theoretically this should not be needed, since
the module
> +               * would be found. *)
> +        | "y" -> true
> +        | _ -> false
> +        )
> +    in
>      let rex_ko_extract = Str.regexp
".*/\\([^/]+\\)\\.k?o\\(\\.xz\\)?$" in
>      let rex_initrd >        if family = `Debian_family then
> @@ -156,7 +171,10 @@ let detect_kernels (g : G.guestfs) inspect family
bootloader >               ) modules in
>               assert (List.length modules > 0);
>  
> -             let supports_virtio = List.mem "virtio_net" modules
in
> +             let kernel_supports what kconf > +               List.mem
what modules || check_config version kconf in
> +
> +             let supports_virtio = kernel_supports "virtio_net"
"VIRTIO_NET" in
>               let is_xen_kernel = List.mem "xennet" modules in
>  
>               (* If the package name is like "kernel-debug", then
it's
> -- 
> 2.7.4
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.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-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Tue, Sep 27, 2016 at 05:58:12PM +0200, Pino Toscano
wrote:
> Detect only once which method must be used to get and set the default
> Grub2 kernel in the guest: this avoids the same checks in list_kernels
> and set_default_kernel.
> 
> Also, add a "no method" option as well: Debian/Ubuntu guests do
not have
> neither grubby nor Perl's Bootloader::Tools, so there is no way to know
> what is the default kernel, nor to change it.  In this case, add a
> warning about this situation.
> ---
>  v2v/linux_bootloaders.ml | 61
++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 17 deletions(-)
> 
> diff --git a/v2v/linux_bootloaders.ml b/v2v/linux_bootloaders.ml
> index 4f53f87..e03d22b 100644
> --- a/v2v/linux_bootloaders.ml
> +++ b/v2v/linux_bootloaders.ml
> @@ -184,6 +184,12 @@ object
>      g#aug_save ()
>  end
>  
> +(** The method used to get and set the default kernel in Grub2. *)
> +type default_kernel_method > +  | MethodGrubby  (** Use the
'grubby' tool. *)
> +  | MethodPerlBootloader  (** Use the 'Bootloader::Tools' Perl
module. *)
> +  | MethodNone  (** No known way. *)
> +
>  (* Grub2 representation. *)
>  class bootloader_grub2 (g : G.guestfs) grub_config >  
> @@ -199,6 +205,20 @@ class bootloader_grub2 (g : G.guestfs) grub_config
>        error (f_"failed to find grub2-mkconfig binary (but Grub2 was
detected on guest)")
>    in
>  
> +  let get_default_method > +    let has_perl_bootloader () > +     
try
> +        ignore (g#command [| "/usr/bin/perl";
"-MBootloader::Tools"; "-e1" |]);
> +        true
> +      with G.Error _ -> false
> +    in
> +    if g#exists "/sbin/grubby" then MethodGrubby
> +    else if has_perl_bootloader () then MethodPerlBootloader
> +    else (
> +      warning (f_"could not determine a way to update the
configuration of Grub2");
> +      MethodNone
> +    ) in
> +
>  object (self)
>    inherit bootloader
>  
> @@ -250,18 +270,23 @@ object (self)
>  
>    method list_kernels >      let get_default_image () > -      let
cmd > -        if g#exists "/sbin/grubby" then
> -          [| "grubby"; "--default-kernel" |]
> -        else
> -          [| "/usr/bin/perl"; "-MBootloader::Tools";
"-e"; "
> -                InitLibrary();
> -                my $default = Bootloader::Tools::GetDefaultSection();
> -                print $default->{image};
> -             " |] in
> -      match g#command cmd with
> -      | "" -> None
> -      | k ->
> +      let res > +        match get_default_method with
> +        | MethodGrubby ->
> +          Some (g#command [| "grubby";
"--default-kernel" |])
> +        | MethodPerlBootloader ->
> +          let cmd > +            [| "/usr/bin/perl";
"-MBootloader::Tools"; "-e"; "
> +                  InitLibrary();
> +                  my $default = Bootloader::Tools::GetDefaultSection();
> +                  print $default->{image};
> +               " |] in
> +          Some (g#command cmd)
> +        | MethodNone ->
> +          None in
> +      match res with
> +      | None -> None
> +      | Some k ->
>          let len = String.length k in
>          let k >            if len > 0 && k.[len-1] =
'\n' then
> @@ -285,10 +310,11 @@ object (self)
>      vmlinuzes
>  
>    method set_default_kernel vmlinuz > -    let cmd > -      if
g#exists "/sbin/grubby" then
> -        [| "grubby"; "--set-default"; vmlinuz |]
> -      else
> +    match get_default_method with
> +    | MethodGrubby ->
> +      ignore (g#command [| "grubby"; "--set-default";
vmlinuz |])
> +    | MethodPerlBootloader ->
> +      let cmd >          [| "/usr/bin/perl";
"-MBootloader::Tools"; "-e"; sprintf "
>              InitLibrary();
>              my @sections = GetSectionList(type=>image,
image=>\"%s\");
> @@ -296,7 +322,8 @@ object (self)
>              my $newdefault = $section->{name};
>              SetGlobals(default, \"$newdefault\");
>            " vmlinuz |] in
> -    ignore (g#command cmd)
> +      ignore (g#command cmd)
> +    | MethodNone -> ()
>  
>    method configure_console = self#grub2_update_console ~remove:false
ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW