On Fri, May 15, 2020 at 10:44:11AM +0300, Denis Plotnikov wrote: > Not all UEFI guests can survive conversion, because of lost bootloader > information in UEFI NVRAM. But some guest can cope with this because they > have a fallback bootloader and use UEFI Removable Media Boot Behavior. > (see https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_A_Feb14.pdf > 3.5.1.1 Removable Media Boot Behavior) to load. If UEFI firmware can't find > a bootloader in its settings it uses the removable media boot behavior to > find a bootloader. > > We can fix the guests which don't have such a fallback loader by providing > a temporary one. This bootloader is used for the first boot only, then the > conversion script restores the initial bootloader settings and removes the > temporary loader.
There's a bunch of both general and specific issues here which I'll try to cover. The biggest general problem is that the code is in the wrong place - it should all be in v2v/convert_linux.ml. I guess it's in v2v/linux_bootloaders.ml because it needed access to grub_config, but you could make a tiny bootloader method bootloader#config_file which returns that value. > Signed-off-by: Denis Plotnikov <[email protected]> > --- > v2v/convert_linux.ml | 15 +++++ > v2v/linux_bootloaders.ml | 149 > ++++++++++++++++++++++++++++++++++++++++++++-- > v2v/linux_bootloaders.mli | 2 + > 3 files changed, 162 insertions(+), 4 deletions(-) > > diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml > index e91ae12..77a2555 100644 > --- a/v2v/convert_linux.ml > +++ b/v2v/convert_linux.ml > @@ -1122,6 +1122,21 @@ shutdown -h -P +0 > Linux.augeas_reload g > ); > > + (* Some linux uefi setups can't boot after conversion because of > + lost uefi boot entries. The uefi boot entries are stored in uefi > + NVRAM. The NVRAM content isn't a part of vm disk content and > + usualy can't be converted with a vm. If a vm doesn't have uefi > + fallback path (/EFI/BOOT/BOOT<arch>.efi), the vm is unbootable > + after conversion. The following function will try to make an uefi > + fallback path if the vm being converted is an uefi setup. > + *) > + > + let efi_fix_script = bootloader#fix_efi_boot () in > + > + if efi_fix_script <> "" then As written, the bootloader#fix_efi_boot method should be returning an option type, ie. Some "firstboot script" or None if one isn't needed. However if this whole function was moved into v2v/convert_linux.ml then it would not need to return anything at all, it could simply install the firstboot script if it was needed, or not install it. > + Firstboot.add_firstboot_script g inspect.i_root > + "fix uefi boot" efi_fix_script; > + > (* Delete blkid caches if they exist, since they will refer to the old > * device names. blkid will rebuild these on demand. > * > diff --git a/v2v/linux_bootloaders.ml b/v2v/linux_bootloaders.ml > index de3d107..cdab7bf 100644 > --- a/v2v/linux_bootloaders.ml > +++ b/v2v/linux_bootloaders.ml ... > +(* Helper function checks if 'source' contains 's' *) > +let contains source s = > + let re = Str.regexp_string s in > + try > + ignore (Str.search_forward re source 0); > + true > + with Not_found -> false > > +(* Helper function to get architecture suffixes for uefi files *) > +let get_uefi_arch_suffix arch = > + let arch_suffix = > + if contains arch "x86_64" then "X64" > + else if contains arch "x86_32" then "X32" > + else "" in > + arch_suffix I don't think you're actually using substrings here? So how about: let get_uefi_arch_suffix = function | "x86_64" -> Some "X64" | "x86_32" -> Some "X32" | None Note again use of the option type. > + > +(* Function fixes uefi boot. It's used in both cases: legacy grub and grub2 > *) > +let fix_uefi g distro distro_ver grub_config arch = > + let cant_fix_uefi () = ( > + info (f_"Can't fix UEFI bootloader. VM may not boot."); > + "" ) in You might want to look at with_return which acts like a return statement: https://github.com/libguestfs/libguestfs-common/blob/e73eca3b73f7d0a54615c5dc55eadd09dc170035/mlstdutils/std_utils.mli#L346 The function would become: let fix_uefi ... = with_return (fun { return } -> if it_can_be_done then ( cant_fix_uefi (); return (); ); if it_can_be_done_for_another_reason then ( cant_fix_uefi (); return (); ); Firstboot.add_firstboot_script ... ) > + let uefi_fallback_name = > + let arch_suffix = get_uefi_arch_suffix arch in > + if arch_suffix <> "" then > + String.concat "" [uefi_fallback_path; "BOOT"; arch_suffix; ".EFI"] > + else > + "" in > + > + if uefi_fallback_name = "" then ( > + info (f_"Can't determine UEFI fallback path."); > + cant_fix_uefi () ) With the return statement and the option typed version of get_uefi_arch_suffix this would become this briefer and much more type-safe code: let uefi_fallback_name = match get_uefi_arch_suffix arch with | None -> cant_fix_uefi (); return () | Some suffix -> sprintf "%sBOOT%s.EFI" uefi_fallback_path suffix in ... > + if file_exists uefi_grub_name && file_exists grub_config then ( This would be a negative test followed by return, ie: if not (file_exists uefi_grub_name) || not (file_exists grub_config) then ( cant_fix_uefi (); return () ); > + g#mkdir_p uefi_fallback_path; > + g#cp uefi_grub_name uefi_fallback_name; > + g#cp grub_config uefi_grub_conf; > + let script = sprintf > +"#!/bin/bash > +efibootmgr -c -L \"CentOS 6\" > +rm -rf %s" uefi_fallback_path in > + script ) Here's an example of a place where you'd simply install the firstboot script. > (* Grub1 (AKA grub-legacy) representation. *) > class bootloader_grub1 (g : G.guestfs) inspect grub_config = > let () = > @@ -60,6 +170,16 @@ class bootloader_grub1 (g : G.guestfs) inspect > grub_config = > fun path -> List.mem_assoc path mounts > ) [ "/boot/grub"; "/boot" ] > with Not_found -> "" in > + > + let uefi_active = > + match inspect.i_firmware with > + | I_UEFI _ -> true > + | _ -> false in > + > + let arch = inspect.i_arch in > + let distro = inspect.i_distro in > + let distro_ver_major = inspect.i_major_version in > + > object > inherit bootloader > > @@ -184,6 +304,12 @@ object > loop paths; > > g#aug_save () > + > + method fix_efi_boot () = > + if uefi_active then > + fix_uefi g distro distro_ver_major grub_config arch > + else > + "" > end Although you don't need any of this code any longer, I will note that splitting the two parts of this function is odd. A simpler way to have written would have been: method fix_efi_boot inspect = match inspect.i_firmware with | I_UEFI -> fix_uefi g inspect.i_distro inspect.i_major_version grub_config inspect.i_arch | I_BIOS -> () Also it's generally better to avoid “| _ ->” where possible since it bypasses type-safety. If we add an extra firmware case in future then the compiler won't tell us that we need to go and check this code to see if it needs an update. 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 _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
