1:39 a.m.
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(a)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/e73eca3b73f7d0a54615...
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