Libguestfs - Nov 2016 - Re: [PATCH v3] v2v: bootloaders: search grub config for all distributions

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

This patch improves the search of grub config on EFI partition. This
means that the config will be found not only for rhel but also for
many other distributions.  Tests were performed on the following
distributions: centos, fedora, ubuntu, suse. In all cases, the config
path was /boot/efi/EFI/*distname*/grub.cfg

The main purpose of the patch is to improve support for converting of
vm with UEFI for most distributions. Unfortunately this patch does not
solve the problem for all distributions, for example Debian does not
store grub config on the EFI partition, therefore for such
distributions another solution is necessary.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 v2v/linux_bootloaders.ml | 78 +++++++++++++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 31 deletions(-)

diff --git a/v2v/linux_bootloaders.ml b/v2v/linux_bootloaders.ml
index e03d22b..0c236e1 100644
--- a/v2v/linux_bootloaders.ml
+++ b/v2v/linux_bootloaders.ml
@@ -49,6 +49,14 @@ let remove_hd_prefix path  
 (* Grub1 (AKA grub-legacy) representation. *)
 class bootloader_grub1 (g : G.guestfs) inspect grub_config +  (* We expect
Augeas to parse the grub_config file.  However
+   * the upstream lens only lists a subset of the possible
+   * locations for this file.  Therefore tell Augeas that
+   * grub_config is a grub file (this apparently doesn't affect
+   * Augeas if it already parsed the file).
+   *)
+  let () = g#aug_transform "grub" grub_config in
+
   (* Grub prefix?  Usually "/boot". *)
   let grub_prefix      let mounts = g#inspect_get_mountpoints inspect.i_root in
@@ -191,7 +199,7 @@ type default_kernel_method    | MethodNone  (** No known
way. *)
 
 (* Grub2 representation. *)
-class bootloader_grub2 (g : G.guestfs) grub_config +class bootloader_grub2 (g :
G.guestfs) inspect grub_config
   let grub2_mkconfig_cmd      let elems = [
@@ -333,34 +341,42 @@ object (self)
     ignore (g#command [| grub2_mkconfig_cmd; "-o"; grub_config |])
 end
 
+(* We can determine if the bootloader config file is grub 1 or
+ * grub 2 just by looking at the filename.
+ *)
+let bootloader_type_of_filename path +  if String.is_suffix path
"/grub.cfg" then
+    Some Grub2
+  else if String.is_suffix path "/grub.conf" ||
+          String.is_suffix path "/menu.lst" then
+    Some Grub1
+  else
+    None
+
+(* Where to start searching for bootloaders. *)
+let bootloader_mountpoint = function
+  | { i_firmware = I_BIOS } -> "/boot"
+  | { i_firmware = I_UEFI _ } -> "/boot/efi/EFI"
+
 let detect_bootloader (g : G.guestfs) inspect -  let config_file, typ -    let
locations = [
-      "/boot/grub2/grub.cfg", Grub2;
-      "/boot/grub/grub.cfg", Grub2;
-      "/boot/grub/menu.lst", Grub1;
-      "/boot/grub/grub.conf", Grub1;
-    ] in
-    let locations -      match inspect.i_firmware with
-      | I_UEFI _ ->
-        [
-          "/boot/efi/EFI/redhat/grub.cfg", Grub2;
-          "/boot/efi/EFI/redhat/grub.conf", Grub1;
-        ] @ locations
-      | I_BIOS -> locations in
-    try
-      List.find (
-        fun (config_file, _) -> g#is_file ~followsymlinks:true config_file
-      ) locations
-    with
-      Not_found ->
-        error (f_"no bootloader detected") in
-
-  match typ with
-  | Grub1 ->
-    if config_file = "/boot/efi/EFI/redhat/grub.conf" then
-      g#aug_transform "grub"
"/boot/efi/EFI/redhat/grub.conf";
-
-    new bootloader_grub1 g inspect config_file
-  | Grub2 -> new bootloader_grub2 g config_file
+  let mp = bootloader_mountpoint inspect in
+  let paths +    try List.map ((^) mp) (Array.to_list (g#find mp))
+    with G.Error msg ->
+      error (f_"could not find bootloader mount point (%s): %s") mp
msg in
+
+  let grub_config, typ +    let rec loop = function
+      | [] -> error (f_"no bootloader detected")
+      | path :: paths ->
+         match bootloader_type_of_filename path with
+         | None -> loop paths
+         | Some typ ->
+            if not (g#is_file ~followsymlinks:true path) then loop paths
+            else path, typ
+    in
+    loop paths in
+
+  (match typ with
+   | Grub1 -> new bootloader_grub1
+   | Grub2 -> new bootloader_grub2) g inspect grub_config
-- 
2.9.3

On Tuesday, 1 November 2016 17:07:04 CET Richard W.M. Jones
wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> This patch improves the search of grub config on EFI partition. This
> means that the config will be found not only for rhel but also for
> many other distributions.  Tests were performed on the following
> distributions: centos, fedora, ubuntu, suse. In all cases, the config
> path was /boot/efi/EFI/*distname*/grub.cfg
> 
> The main purpose of the patch is to improve support for converting of
> vm with UEFI for most distributions. Unfortunately this patch does not
> solve the problem for all distributions, for example Debian does not
> store grub config on the EFI partition, therefore for such
> distributions another solution is necessary.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  v2v/linux_bootloaders.ml | 78
+++++++++++++++++++++++++++++-------------------
>  1 file changed, 47 insertions(+), 31 deletions(-)
> 
> diff --git a/v2v/linux_bootloaders.ml b/v2v/linux_bootloaders.ml
> index e03d22b..0c236e1 100644
> --- a/v2v/linux_bootloaders.ml
> +++ b/v2v/linux_bootloaders.ml
> @@ -49,6 +49,14 @@ let remove_hd_prefix path >  
>  (* Grub1 (AKA grub-legacy) representation. *)
>  class bootloader_grub1 (g : G.guestfs) inspect grub_config > +  (* We
expect Augeas to parse the grub_config file.  However
> +   * the upstream lens only lists a subset of the possible
> +   * locations for this file.  Therefore tell Augeas that
> +   * grub_config is a grub file (this apparently doesn't affect
> +   * Augeas if it already parsed the file).
> +   *)
> +  let () = g#aug_transform "grub" grub_config in
> +
>    (* Grub prefix?  Usually "/boot". *)
>    let grub_prefix >      let mounts = g#inspect_get_mountpoints
inspect.i_root in
> @@ -191,7 +199,7 @@ type default_kernel_method >    | MethodNone  (** No
known way. *)
>  
>  (* Grub2 representation. *)
> -class bootloader_grub2 (g : G.guestfs) grub_config > +class
bootloader_grub2 (g : G.guestfs) inspect grub_config >
>    let grub2_mkconfig_cmd >      let elems = [
> @@ -333,34 +341,42 @@ object (self)
>      ignore (g#command [| grub2_mkconfig_cmd; "-o"; grub_config
|])
>  end
>  
> +(* We can determine if the bootloader config file is grub 1 or
> + * grub 2 just by looking at the filename.
> + *)
> +let bootloader_type_of_filename path > +  if String.is_suffix path
"/grub.cfg" then
> +    Some Grub2
> +  else if String.is_suffix path "/grub.conf" ||
> +          String.is_suffix path "/menu.lst" then
> +    Some Grub1
> +  else
> +    None
Our String.is_suffix is a does String.sub every time, so this is a bit
inefficient -- I'd use Common_utils.last_part_of:

  let bootloader_type_of_filename path     match last_part_of path '/'
with
    | Some "grub.cfg" -> Some Grub2
    | Some ("grub.conf"|"menu.lst") -> Some Grub1
    | Some _ | None -> None
> +(* Where to start searching for bootloaders. *)
> +let bootloader_mountpoint = function
> +  | { i_firmware = I_BIOS } -> "/boot"
> +  | { i_firmware = I_UEFI _ } -> "/boot/efi/EFI"
It seems to be used only once, so I'd just put it inline below (it's
small to read).
> +
>  let detect_bootloader (g : G.guestfs) inspect > -  let config_file, typ
> -    let locations = [
> -      "/boot/grub2/grub.cfg", Grub2;
> -      "/boot/grub/grub.cfg", Grub2;
> -      "/boot/grub/menu.lst", Grub1;
> -      "/boot/grub/grub.conf", Grub1;
> -    ] in
> -    let locations > -      match inspect.i_firmware with
> -      | I_UEFI _ ->
> -        [
> -          "/boot/efi/EFI/redhat/grub.cfg", Grub2;
> -          "/boot/efi/EFI/redhat/grub.conf", Grub1;
> -        ] @ locations
> -      | I_BIOS -> locations in
> -    try
> -      List.find (
> -        fun (config_file, _) -> g#is_file ~followsymlinks:true
config_file
> -      ) locations
> -    with
> -      Not_found ->
> -        error (f_"no bootloader detected") in
> -
> -  match typ with
> -  | Grub1 ->
> -    if config_file = "/boot/efi/EFI/redhat/grub.conf" then
> -      g#aug_transform "grub"
"/boot/efi/EFI/redhat/grub.conf";
> -
> -    new bootloader_grub1 g inspect config_file
> -  | Grub2 -> new bootloader_grub2 g config_file
> +  let mp = bootloader_mountpoint inspect in
> +  let paths > +    try List.map ((^) mp) (Array.to_list (g#find mp))
> +    with G.Error msg ->
> +      error (f_"could not find bootloader mount point (%s): %s")
mp msg in
> +
> +  let grub_config, typ > +    let rec loop = function
> +      | [] -> error (f_"no bootloader detected")
> +      | path :: paths ->
> +         match bootloader_type_of_filename path with
> +         | None -> loop paths
> +         | Some typ ->
> +            if not (g#is_file ~followsymlinks:true path) then loop paths
> +            else path, typ
The g#is_file is there to exclude non-files (like directories), right?
Otherwise it would be weird for g#find to return paths which don't
exist..

LGTM otherwise.

Thanks,
-- 
Pino Toscano

On Tue, Nov 01, 2016 at 06:29:06PM +0100, Pino Toscano
wrote:
> Our String.is_suffix is a does String.sub every time, so this is a bit
> inefficient -- I'd use Common_utils.last_part_of:
> 
>   let bootloader_type_of_filename path >     match last_part_of path
'/' with
>     | Some "grub.cfg" -> Some Grub2
>     | Some ("grub.conf"|"menu.lst") -> Some Grub1
>     | Some _ | None -> None
OK, I'll change that.  I should note that v3 of the patch also fails
check-slow for reasons I'm investigating.

[...]
> The g#is_file is there to exclude non-files (like directories), right?
> Otherwise it would be weird for g#find to return paths which don't
> exist..
Right, it's so we don't continue with a directory on some strange
system.  I was considering adding a warning.

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

It turns out that using grub_transform unconditionally causes the
grub.conf parse to fail with:

  augeas failed to parse /boot/grub/grub.conf:
   error "Lenses @Grub and grub.lns could be used to load this file"

I think that means that two "different" lenses apply to the file which
confuses Augeas (see src/transform.c:transform_load).

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