Bug#922251: live-build: support syslinux-efi as (additional) bootloader
El 16/04/19 a las 20:17, Matthijs Kooijman escribió:
> Hi Adrian,
>
> thanks for your extensive review. I'll respond inline.
>
>> I might take a look into your notes to implement grub-efi + secure boot
>> in hdd img but... this might be in 2021 XD . Too busy at the moment.
> Familiar sentiment. Would be nice to have it, though.
>
>> What's your use case? What do you need to build live-build based hdd
>> imgs? I'm curious.
> I'm using live-build to build Webconverger (a kiosk OS booting into a
> full-screen browser) and an image derived for that for Bizplay (a
> digital signage system).
>
> We need hdd images rather than isohybrid images, so the resulting
> USB-disk can be written to. This is needed firstly to update the
> bootloader configuration to change the kernel commandline (we use this
> to customize the image, e.g. change the default homepage shown).
> Secondly, the image uses git to manage the root filesystem and a running
> image can update itself, but that again needs a writable filesystem.
Ok, understood.
>>> As predicted by others in this bug report, my work does not enable
>>> secure boot (which syslinux simply does not support), nor enable
>>> syslinux-efi support in iso/isohybrid images (since syslinux-efi does
>>> not support this, or at least it apparently does not work).
>>
>> Nice. What you need to make sure is that you cannot choose syslinux-efi
>> when trying to build iso/isohybrid images.
> Yeah, that's checked by the "Improve bootloader configuration checks"
> commit.
Ok, I saw that.
>>> 0) backward incompatible change
>> In the pull request there is a mention about a backward incompatible
>> change. Can you please describe in what that change consists?
> I've added one commit with a NEWS file that describes this issue in a
> bit more detail. Does that clarify sufficiently?
>
> Note that while writing the NEWS entry, it seemed more confusing to have
> a bootloaders/syslinux-common directory as well as a syslinux-common
> Debian package, so I ended up modifying my commits to use
> "syslinux-shared" rather than "syslinux-common" (I'll also use the
> former below).
Ok, now 'Add NEWS entry about syslinux config refactoring'
(00eab3a7) makes clear what it's not backward compatible (under certain
circumstances).
It's ok for me.
>>> 1) Refactor some bootloader scripts (969cdf4e)
>>
>> 1.1) Nice idea the Get_Bootloader_Config_Dir function.
>> Have you made sure that $BOOTLOADER variable is not used anywhere else
>> in the code?
>> Maybe you should make that variable local.
> Good point. I guess I was following the style of other functions (that
> dod not use local variables either), but that's no reason not to do it
> here. I updated the PR with this change.
Nice.
>> 1.2) The function Install_Bootloader_Files does not seem to be right.
>> If you are not going to reuse that function in more than one place (I'll
>> check your other commits later) you might consider not having that
>> function in the first place.
> I am indeed planning to reuse the function. I've clarified this in the
> commit message.
Then it's ok I guess.
>> Even if the Install_Bootloader_Files function was a good idea it's not
>> well implemented commit-wise. You are performing two changes. One about
>> moving the functionality to a function. Another one is chaning the
>> comment about "two steps" to "three steps". It's hard to notice if you
>> changed something other than that comment on that commit.
>>
>> I mean... this part should be splitted in two commits.
> I'm usually a nitpick in this area, but here I decided for a bit more
> simplicity. You're right, though. I ended up splitting this into three
> commits, one for Get_Bootloader_Config_Dir, one for
> Install_Bootloader_Files and one for the comment.
Ok. Thank you.
>>> 2) Reduce config duplication in syslinux variants (bccd127b)
>>
>> I know it's better the way you merge files there but I think the
>> original idea was about having independent configuration files for each
>> different media.
>>
>> I mean there were not meant to be merged together in the first place.
> The reason to do this, is that I want configuration files to be shared
> between syslinux and syslinux-efi (which can co-exist in the same
> image). If these would be duplicated everywhere, modifying the
> bootloader config after an image is written to disk requires modifying
> it in two places.
>
> Note that syslinux-efi could still not have its own config files, but
> refer to files contained in bootloaders/syslinux (without the need for
> bootloaders/syslinux-shared), but then you can never have an image that
> *only* contains syslinux-efi (which you can with the syslinux-shared
> approach, since then you use syslinux-shared for the config and
> syslinux-efi for the efi images).
Ok, I guess it's fine. live-wrapper produced ISOs from official Debian
also have those common and shared cfg files I guess that stolen from the
way Ubuntu (and other distros) deal with this too.
>>> 3) Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)
>>
>> 3.1) Nice. I didn't know about those new syslinux architecture dependent
>> files. As per the wiki you link (
>> https://wiki.syslinux.org/wiki/index.php?title=Library_modules#All_Syslinux_variants_need_an_additional_ldlinux_module
>> ) in the commit description I guess that even ldlinux.c32 wouldn't be
>> used but ldlinux.e64 instead.
> Actually, ldlinux.e64 is only for EFI. This commit only touches
> BIOS-related stuff. I'm not sure how "architecture dependent files" are
> relevant here, since this commit just removes some files which are
> really superfluous (since the 'syslinux' and 'extlinux' commands used to
> install the bootloader in binary_hdd make sure to copy their own version
> of these files into the image).
I'm revisiting this commit and I'm not sure this is right thing to do.
If your pull request only affects syslinux-efi why do you even care
about 32bit code?
You are assuming that syslinux and extlinux commands are only used in
binary_hdd and by grepping the code it's seems to be true. So it's ok
with this assumption.
I guess that this directory was initially meant to be used as a template.
So it makes sense to keep those c32 files into their original place.
Just in case you want to override these files with your own custom c32
files.
Is it worth keeping those files there nowadays?
They are just symlinks after installing the package.
Well, I'm not sure about this one commit.
Anyway I think this commit should be on its own Merge Request because I
think adding syslinux-efi is going to work regardless of these files
getting into the final disk.
>> 3.2) BTW Do you support both EFI IA32 and EFI X64 or only EFI X64 ?
> Yes, but that's a later commit.
Ok.
>>> 4) Move syslinux modules into subdirectories (53901001)
>>
>> Shouldn't those c32 modules be moved to a folder named c32-modules (or
>> c32mods for short name) instead?
>>
>> Let's see how you name the UEFI modules folder later.
>>
>> In "Add syslinux-efi bootloader support (a271f8f9)" you use modules32
>> folder name for some c32. Shouldn't you use modules32 as a folder name
>> in this "Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)" commit
>> in the first place?
>>
>> I'm not sure about this one I might be missing something.
> I think the missing bit might be that
> bootloaders/{syslinux,syslinux-shared} is installed into binary/boot,
> while bootloaders/syslinux-efi is installed into binary/EFI/boot, so
> these are distinct locations.
You are right. This was the missing bit. I see.
> Also, EFI supports 32 and 64 bit (hence modules32 and modules64), but
> BIOS is (by definition, I think) only 32-bit, so I just used "modules".
Well, I think you should use something else.
modules32 is 9 characters long (not 8.3 compatible). What about module32
and module64? So that we can reuse the code in the iso side when
isolinux is updated to support hybrid isos in efi mode ?
>>> 5) Add syslinux-efi bootloader support (a271f8f9)
>> 5.1) You have got a nice bug on that code XD . Just after the Echo_error
>> statements about syslinux-efi not being supported there should be an
>> exit 1 statement.
> Thanks, fixed.
Nice.
>> 5.2) Anyway I don't think I like this code at all. I mean... you are
>> supposed to create a new file named:
>> scripts/build/binary_syslinux-efi
>> and not hack around the existant one: binary_syslinux
>
> That would make sense if I wanted syslinux-efi to be really indepenent,
> but as I noted above, I wanted to make them share a single configuration
> (and also allow syslinux-efi to be installed by itself). This seemed
> like best way to achieve that, though alternative suggestions are
> welcome :-)
Well, you could have a binary_syslinux_cfg file similar to the
binary_loopback_cfg one or maybe binary_shared_cfg.
>
> I pushed a new version with the above changes to
> https://salsa.debian.org/live-team/live-build/merge_requests/19
6.1)
PATH syslinux command is still being written in capital letters in
share/bootloaders/syslinux-efi/syslia32.cfg and
share/bootloaders/syslinux-efi/syslx64.cfg while it should be written in
non-capital letters.
6.2) What you should learn about the multibootloader support is that it
should let use as much as bootloaders as you want to.
So, you could have something like:
--bootloaders syslinux,syslinux-efi,grub-efi
which, of course it does not make sense because either syslinux-efi and
grub-efi would overwrite each ones code.
Anyway what I wanted to say here is that:
--bootloaders syslinux,syslinux-efi
and
--bootloaders syslinux-efi,syslinux
are not the same thing.
The first one (regarding binary_syslinux) should install special MBR
code so that BIOS can chainload into syslinx.
The second one (regarding binary_syslinux) should NOT install special
MBR code so that BIOS can chainload into syslinux. It should inhibit
itself because it is not the first bootloader.
Or maybe it should not inhibit but you can do it.
Well, I only had to deal with binary_iso when adding multi bootloaders
support so I modified the binary_hdd the minimum so that it worked with
the new variables.
So for the binary_iso part we used:
https://salsa.debian.org/live-team/live-build/blob/e4a4d4ae8d6379eae39eacb95486df1e5f028a69/scripts/build/binary_iso#L124-127
.
if [ ${BOOTLOADER_NUMBER} -ge 2 ]
then
XORRISO_OPTIONS="${XORRISO_OPTIONS} -eltorito-alt-boot "
fi
This means that an extra 'eltorito' is added, so that the new entry at
the isos can be recognised.
So that a BIOS firmware that supports eltorito specification can choose
between booting into the default torito entry or the non default one.
I'm not sure there's an equivalent thing that should be added to
binary_hdd now that we are at it.
Anyways... if we take a look at your untouched binary_hdd:
https://salsa.debian.org/live-team/live-build/blob/00eab3a77f3da176f3f0aa807b886206f8f0f0f1/scripts/build/binary_hdd
we can see that grub-pc is not supported. I guess I might add it in the
future.
But grub legacy is supported. (Well, it's not actually not supported.)
Assuming grub legacy are available at buster which I'm not totally sure.
Anyway... what I want to say is that you should be able to choose:
--bootloaders grub,syslinux-efi
or (if grub-pc was implemented there)
--bootloaders grub-pc,syslinux-efi
in a hdd target and 'syslinux' installation should be only triggered if
it's the first bootloader.
Well, that's exactly how binary_hdd works right now... although the
multi bootloader part should be improved to have something better than:
https://salsa.debian.org/live-team/live-build/blob/00eab3a77f3da176f3f0aa807b886206f8f0f0f1/scripts/build/binary_hdd#L60-86
All of this above is trying to improve multi bootloader support in the
binary_hdd part of live-build. Not sure if you should deal with this
prior to adding your code.
But if you want to take a look and tell us if binary_hdd should be
updated or not (in the end the efi installation is handled by
binary_syslinux-efi or binary_grub-efi in the filesystem level and not
by binary_hdd).
Having to deal with separate bootloaders, what they add or contribute
that's another reason why I prefer binary_syslinux and
binary_syslinux-efi being in different files.
6.3) Many of your commits seem to need a rebase into the current master
branch. Well, that's to be expected.
>
> Gr.
>
> Matthijs
>
adrian15
--
Support free software. Donate to Super Grub Disk. Apoya el software
libre. Dona a Super Grub Disk. http://www.supergrubdisk.org/donate/
Reply to: