Libguestfs - Apr 2017 - Re: [PATCH v6 1/1] v2v: bootloaders: search grub config for all distributions

On Wednesday, 19 April 2017 15:58:57 CEST Pavel Butsykin
wrote:
> 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 | 86
+++++++++++++++++++++++++++++++-----------------
>  1 file changed, 55 insertions(+), 31 deletions(-)
> 
> diff --git a/v2v/linux_bootloaders.ml b/v2v/linux_bootloaders.ml
> index cad72a829..593797f4e 100644
> --- a/v2v/linux_bootloaders.ml
> +++ b/v2v/linux_bootloaders.ml
> @@ -42,6 +42,10 @@ type bootloader_type >    | Grub1
>    | Grub2
>  
> +let string_of_bootloader_type = function
> +  | Grub1 -> "Grub1"
> +  | Grub2 -> "Grub2"
> +
>  (* Helper function for SUSE: remove (hdX,X) prefix from a path. *)
>  let remove_hd_prefix path >    let rex = Str.regexp
"^(hd.*)\\(.*\\)" in
> @@ -49,6 +53,13 @@ let remove_hd_prefix path >  
>  (* Grub1 (AKA grub-legacy) representation. *)
>  class bootloader_grub1 (g : G.guestfs) inspect grub_config > +    let
()
Wrong indentation here.
> +    (* Apply the "grub" lens if it is not handling the file
> +     * already -- Augeas < 1.7.0 will error out otherwise.
> +     *)
> +    if g#aug_ls ("/files" ^ grub_config) = [||] then
> +      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 +202,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
NACK, see below.
>  
>    let grub2_mkconfig_cmd >      let elems = [
> @@ -335,33 +346,46 @@ object (self)
>  end
>  
>  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
> +  (* Where to start searching for bootloaders. *)
> +  let mp > +    match inspect.i_firmware with
> +    | I_BIOS -> "/boot"
> +    | I_UEFI _ -> "/boot/efi/EFI" in
> +
> +  (* Find all paths below the mountpoint, then filter them to find
> +   * the grub config file.
> +   *)
> +  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
> +
> +  (* 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 > +    match last_part_of path
'/' with
> +    | Some "grub.cfg" -> Some Grub2
> +    | Some ("grub.conf" | "menu.lst") -> Some Grub1
> +    | Some _
> +    | None -> None
> +  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
> +
> +  debug "detected bootloader %s at %s"
> +        (string_of_bootloader_type typ) grub_config;
You don't need string_of_bootloader_type, there's already the #name
method of the bootloader object.  The only change would be moving this
message after the creation of the bootloader subclass.

Another option is adding a new virtual method to the bootloader object,
something like output#as_options, e.g.
  method debug = "grub1(" ^ grub_config ^ ")"
so you can change the code to be:

  let bl     match typ with
    | Grub1 -> new bootloader_grub1 g inspect config_file
    | Grub2 -> new bootloader_grub2 g config_file in
  debug "detected bootloader: %s" bl#debug;
> +  (match typ with
> +   | Grub1 -> new bootloader_grub1
> +   | Grub2 -> new bootloader_grub2) g inspect grub_config
Just leave the current way, which is more readable.

Thanks,
-- 
Pino Toscano