You noted in the patch: "Defaults to yes because we do not do any library
checking for now."
With this patch you tell SWUpdate libubootenv is available, unconditionally,
which might as well not be the case, right? Then, you tell SWUpdate it's
available while it is not?
[see bottom comment first]
> > > Well we never really hit the EBG issue since buildroot hasn't ever packaged
> > > efibootguard at all. I guess we probably should reintroduce the
> > > HAVE_LIBEBGENV variable in case we add efibootguard support in the
> > > future.
> >
> > You will run into the same problem as EFI Boot Guard integration works
> > alike. Eventually, we may probably also have others like zchunk or rsync
> > run-time configurable and then we have the problem again.
> > So, ideally, the HAVE_ options should be set depending on the SWUpdate
> > options chosen and the available libraries on the build/target system at
> > build-time. If they don't match, it doesn't build.
> > This would also solve your problem I guess.
>
> The purpose of the HAVE_ options is so that buildroot can tell swupdate
> what options swupdate can enable when running swupdate-menuconfig
> inside of buildroot and so that swupdate's menuconfig can say what is
> missing when being run by the user using buildroot's configure system.
OK, but then ― if I got this correctly ― you need to have some sort of
library checking in SWUpdate to match SWUpdate's view to Buildroot's?
If enabled unconditionally, SWUpdate will present the libubootenv
option, always, without ever noting what's missing?
[see bottom comment first]
> > > > > (1) Port those external type definition to the bootloader binding
> > > > > implementations in SWUpdate (need to be maintained!) or
> > > >
> > > > We duplicate code and we go into trouble if the libraries are changing
> > > > the API.
> >
> > Right, and it's work on SWUpdate's side that can be avoided, agreed.
> >
> >
> > > For buildroot runtime and compile time libraries should always be the
> > > same as we don't really support binary package installation.
> > >
> > > I think it may make sense to have an option to disable dlopen library
> > > loading entirely as this feature only seems useful for systems that
> > > support binary package installation.
> >
> > Exactly this was the purpose: Enable bootloader selection at run-time by
> > configuration rather than statically at compile-time. This actually
> > enables SWUpdate to be shipped as a package by (binary) distributions
> > with probably support for multiple bootloaders enabled.
> > Then, you have to prepare an SWUpdate package per architecture and just
> > configure it to use the right bootloader (out of those whose support is
> > enabled).
>
> I'm not saying it's not useful for some cases, but as embedded systems
> that use swupdate often handle updates simply by replacing the entire
> rootfs it's likely not something needed in a lot of setups as they will
> only be updating swupdate by replacing the rootfs and not individually.
Yes, but it's not that uncommon to build the rootfs with a meta-distribution
based on binary packages, e.g., ELBE, ISAR, ..., for good reasons.
For (binary) distributions, if compile-time tying the bootloader to the
SWUpdate binary, you would have to supply binary packages for all
combinations, i.e., swupdate-<bootloader>-<arch>.pkg. With this feature,
you just have to make *one* package swupdate-<arch>.pkg and configure
the bootloader to use. It's a big win for (binary) distributions with
respect to maintainability, testing efforts, etc. at IMO no cost for the
"classic" embedded use cases.
We do exactly this for many products: Use a binary meta-distribution and
replace the rootfs in an A/B deployment.
> > If you disable this feature, then you're back at specifically building
> > SWUpdate packages for each architecture + target combination.
> > While this may work (better?) for Buildroot, it destroys the (binary)
> > distribution use case, e.g., Debian.
>
> I'm just suggesting a config option to allow it to be built with dlopen
> library loading disabled. Buildroot isn't a binary distribution so it's
> just added complexity and may potentially allow say missing library
> issues to go undetected for longer.
Hm, this would then require maintaining two bindings to each bootloader:
one for the "static" linkage and one for dlopen(). If Buildroot tells
SWUpdate what's available (and installs this to the target) and SWUpdate
would only use what Buildroot told it to use, then I don't see a problem?
> > Even so, with the implemented probing, you do have the same run-time and
> > compile-time libraries: They're just loaded differently (dlopen() vs.
> > ldlinux). If you chose at compile-time to just use U-Boot, then only
> > U-Boot support is enabled and is the default, i.e., libubootenv is
> > dlopen()'d on SWUpdate startup.
> >
> > So I do not see a benefit in introducing an option to disable this
> > feature. Did I overlook one?
>
> The issue I think is that ldlinux loading ensures that if the program runs
> all expected libraries are available while dlopen tends to be more likely
> to fail later, it's basically more brittle.
In principle yes, and I would call this a feature for other use cases ;)
Anyway, that's why we use dlopen() with RTLD_NOW and load the libraries
quite early in the startup of SWUpdate. So, if it fails, it fails early
upfront and not while first using the bootloader functionality.
This was very important to get similar fail-early behavior like when
using ldlinux. Granted, now it fails a bit later but still prior to
SWUpdate normal operation, i.e., while SWUpdate's initialization, and
even gives you a nice(r) error message ;)
For example, this is what I get when I move libebgenv.so.0 out of the way:
[TRACE] : SWUPDATE running : [print_registered_bootloaders] : Registered bootloaders:
[TRACE] : SWUPDATE running : [print_registered_bootloaders] : uboot loaded.
[TRACE] : SWUPDATE running : [print_registered_bootloaders] : none loaded.
[TRACE] : SWUPDATE running : [print_registered_bootloaders] : ebg shared lib not found.
[ERROR] : SWUPDATE failed [0] ERROR core/swupdate.c : main : 779 : Default bootloader interface 'ebg' couldn't be loaded.
[INFO ] : SWUPDATE running : [main] : Check that the bootloader interface shared library is present.
[INFO ] : SWUPDATE running : [main] : Or chose another bootloader interface by supplying -B <loader>.
> Also static linking doesn't work with dlopen(although not sure swupdate
> really supports that properly in general).
Well, at least not without some extra work (post-processing the ELFs
which is possible in this case as they're not random run-time plugins
but defined bindings) or losing the benefits you're after when
statically linking by allowing the "cruft" to creep in via loadable
modules again: You have to initialize ld.so for libdl to properly
execute dlopen() and this without symbol clashes. It does work but
it's an overly ugly hack just for proofing the concept.
OK, so you kind of "mimic" something like pkgconf by the ENV_ variables
which define the available and to be built libraries at the later build
stage. Hence, you cannot detect while SWUpdate menuconfig whether a
particular library is/will be present or not. Hence, you set libubootenv
and thereby convey to Buildroot (?) to install libubootenv at the later
build stage. If so, then introducing ENV_libebgenv would also enforce
to build/install libebgenv which might be unused as you've opted for
using libubootenv?
> > > > My point here (apart having the same behavior for all bootloaders, that
> > > > is EBG, too) is if this patch has drawbacks that I do not understand /
> > > > see, because these HAVE_ were explicitely removed with the commit above.
> >
> > They were removed since you select to compile in support for bootloaders
> > at run-time. This doesn't necessarily mean that there actually is the
> > proper library on the target system. So you have to configure it on the
> > target to chose the bootloader for which (1) SWUpdate has support
> > compiled in and (2) whose support library is actually installed.
>
> Yeah, in our case though our build configuration matches the runtime
> in effectively all use cases as we don't build binary package artifacts.
OK, as this is in sync, I think there's no problem with the dlopen()
solution per se, right?
> > Relying on the HAVE_ options all turned on means you always build
> > support for all bootloaders ― which is not what you may want?
>
> We still allow the user to configure the options, we just use the HAVE_
> variables to restrict what can be configured.
OK, if we HAVE_libubootenv and HAVE_libebgenv, then both are build but
only one is used, the one which is ticked in SWUpdate's menuconfig?
Thanks for bearing with me getting Buildroot's inner workings straight
in my mind! ;)