[Libguestfs] [PATCH] v2v: fix UEFI bootloader for linux guests
Richard W.M. Jones
rjones at redhat.com
Sat Jun 6 06:39:30 UTC 2020
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 <dplotnikov at virtuozzo.com>
> ---
> 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
More information about the Libguestfs
mailing list