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

On Fri, Apr 28, 2017 at 07:57:56PM +0300, Pavel Butsykin
wrote:
> On 28.04.2017 17:45, Pino Toscano wrote:
> >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.
> 
> This is not so important point for me, I'll make the following change
> in the next patch:
> 
>   let bl >     (match typ with
>      | Grub1 -> new bootloader_grub1 g inspect grub_config
>      | Grub2 -> new bootloader_grub2 g grub_config) in
>   debug "detected bootloader %s at %s" bl#name grub_config;
>   bl
> 
> But these changes did Rich, I can submit the next patch if all
> satisfied with the above suggested option.
> 
> Rich?
Sure, whatever Pino thinks is fine.

Rich.

-- 
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/