[Libguestfs] [PATCH v3] v2v: bootloaders: search grub config for all distributions

[Pino Toscano]

On Tuesday, 1 November 2016 17:07:04 CET Richard W.M. Jones wrote:
> From: Pavel Butsykin <pbutsykin at 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 at virtuozzo.com>
> Signed-off-by: Richard W.M. Jones <rjones at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20161101/2e32df7b/attachment.sig>