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

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(a)virtuozzo.com&gt; >>Signed-off-by: Richard W.M. Jones <rjones(a)redhat.com&gt; >>--- >> 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?