#813 PR `closed`: determine EFI virtual disk size automatically¶

Labels: enhancement, bug, cleanup

gozora opened issue at 2016-04-03 15:12:¶

Fix for manual detection of EFI virtual image (efiboot.img) size.

jsmeix commented at 2016-04-04 08:23:¶

@gozora
many thanks for the fix!

It looks much cleaner now (at least for me).

In particular I prefer to have that single task now
in one single script (including its mount and umount)
and no longer split into several scripts.

In 70_create_efibootimg.sh I wonder about the commented out code

#mv $v -f $TMP_DIR/efiboot.img $TMP_DIR/boot/efiboot.img >&2
...
#ISO_FILES=( ${ISO_FILES[@]} $TMP_DIR/boot/efiboot.img )

which is a copy from the old 70_umount_efibootimg.sh
because I do not understand why that commented out code
is there.
It looks as if it is it meant as an alternative way how to do it
but then I would like to know the pros and cons of both ways.

An addendum only FYI because the following
is a matter of personal preference
how one likes to implement it:

In general I wonder when a function (like efiboot_img_size)
is only called in one single script if then the function
should be better also defined only in that script?
I.e. if a function that is "local" for a single script
shouldn't be better defined locally in that script?

And even a step further:
I wonder if a function is needed at all when
some functionality is only needed one single time.
Why not have such functionality implemented
directly "inline"?

Of course it can make code better readable when
certain tasks are split out into function definitions
even if each task is done only once.

But I think the efiboot_img_size functionality is
sufficiently small and simple to be also readable
when its code would be directly "inline"
in the 70_create_efibootimg.sh script.

Again: This is a matter of personal preference
how one likes to implement it.

jsmeix commented at 2016-04-04 08:28:¶

I forgot my main question:

Before the minimum EFI virtual image size was 32000 KiB
(via "size=32000").

Now the minimum EFI virtual image size is 128 MiB.

Why?

As far as I understand it this could make the ISO image
of the rear recovery system needlessly bigger.

Some users use rear on thousands of their servers
and therefore they must store thosands of images
of the rear recovery system (one for each server)
and then about 100MiB more for each image can
make a noticeable difference for them.

In other words:

When the exact right size of EFI virtual image is determined,
why is there a minimum EFI virtual image size at all?

Why not always use the exact right size of EFI virtual image?

gozora commented at 2016-04-04 08:55:¶

For the functions I just followed what was already written (I'm lazy person and it is more convenient like this), but I share your view. Local functions should be local, so if you prefer to have function directly in 70_create_efibootimg.sh, in can move it there.

The size topic is a bit tricky, but to cut the story short, smaller (32MB) image size is fine when GRUB is used, (BTW. now I've realized that this check was committed, I'll correct in in next pull request with something similar to this). As GRUB is capable to "escape" virtual image into ordinary ISO filesystem, and read kernel and initrd from there. ELILO is not, so you must copy kernel together with initrd into image which makes it significantly bigger.

And one interesting fact that I've learned the hard way; virtual image must be aligned to 32MB ...

jsmeix commented at 2016-04-04 11:06:¶

Regarding the efiboot_img_size function just do it as you like
(i.e. it is perfectly fine to be lazy and leave it as is).

Regarding the size:
I totally trust you because you are the expert here.

FYI regarding https://github.com/rear/rear/blob/ce01a145451f74f15c6610e8611d60cbc1ce360a/usr/share/rear/output/ISO/Linux-i386/20_mount_efibootimg.sh

I wonder if there could be perhaps a bug because MB != MiB
and perhaps it must be MiB (1024 x 1024)
and not MB (1000 x 1000)?

   size=128000
else
   size=32000
...
dd if=/dev/zero of=$TMP_DIR/efiboot.img count=$size bs=1024

does neither result a 128 MiB nor MB nor 32 MiB nor MB efiboot.img

Is 128 x 1000 x 1024 or 32 x 1000 x 1024 really correct
or must it be 128 x 1024 x 1024 and 32 x 1024 x 1024 for MiB
or 128 x 1000 x 1000 and 32 x 1000 x 1000 for MB?

Cf.
https://en.wikipedia.org/wiki/Byte#Unit_symbol

Accordingly 1000 x 1024 would be kKiB ;-)

jsmeix commented at 2016-04-04 11:46:¶

Sorry, I got a bit confused

I see that
https://github.com/rear/rear/blob/ce01a145451f74f15c6610e8611d60cbc1ce360a/usr/share/rear/output/ISO/Linux-i386/20_mount_efibootimg.sh is old code.

In the new code that uses

dd ... count=$(efiboot_img_size $TMP_DIR/mnt) bs=1M

the kKiB versus MiB issue is fixed.

But I found a kKiB versus MiB issue still in
usr/share/rear/output/ISO/Linux-ia64/20_mount_bootimg.sh

dd if=/dev/zero of=$TMP_DIR/boot.img count=64000 bs=1024

"git blame" shows that @gdha made that one.

@gdha
should
usr/share/rear/output/ISO/Linux-ia64/20_mount_bootimg.sh
be fixed to

dd if=/dev/zero of=$TMP_DIR/boot.img count=64 bs=1M

gozora commented at 2016-04-04 12:43:¶

I've just pushed a commit about "near to be finished patch".
However I can't imagine in my head about possible site effect so I need to test if first.
Once I have tests finished I'll create another pull request.
BTW. I've moved efiboot_img_size() directly to 70_create_efibootimg.sh

gdha commented at 2016-04-04 12:52:¶

Do not bother the Linux-ia64 scripts - these systems are getting obsolete anyway

gozora commented at 2016-04-04 15:45:¶

I've done some testing (unfortunately only on my virtual guests) and all seems to be working fine.
If you have some better test possibilities (some real HW), please try to test this patch before merging with main tree.