[OpenWrt-Devel] [PATCH 3/3] x86: add preinit hook for bootloader upgrade

Tomasz Maciej Nowak tomek_n at o2.pl
Mon Jun 10 10:28:49 EDT 2019


Hi Petr.

W dniu 27.05.2019 o 18:17, Petr Štetiar pisze:
> Tomasz Maciej Nowak <tomek_n at o2.pl> [2019-05-27 14:46:30]:
> 
> Hi,
> 
>> new file mode 100644
>> index 0000000000..3a4e756b1e
>> --- /dev/null
>> +++ b/target/linux/x86/base-files/lib/preinit/81_upgrade_bootloader
> 
> makes me wonder if this couldn't be made more generic as for example current
> sysupgrade.tgz restoration, maybe adding platform_do_bootloader_upgrade hook
> which would get called in some generic preinit script or something along these
> lines.  Otherwise we're likely risking copy&pasting of similar functionality
> to other platforms.

There won't be any platform using this, it's only to fill the lack of bootloader
upgrade for current x86 OpenWrt installations. All other targets had bootloader
upgrade on sysupgrade since inception (the targets for which OpenWrt provides
bootloader). We can wait for another release which will certainly upgrade the
bootloader (if patch #2 will get accepted) or do it now, and delete the preinit
upgrade from master, when we will stop support for the release when this was first
added.

> 
>> +upgrade_bootloader() {
>> +	local diskdev
>> +
>> +	. /lib/upgrade/common.sh
>> +
>> +	if [ ! -f /boot/grub/upgraded ] && export_bootdevice && export_partdevice diskdev 0; then
>> +		echo "(hd0) /dev/$diskdev" > /tmp/device.map
>> +		/usr/sbin/grub-bios-setup \
>> +			-m "/tmp/device.map" \
>> +			-d "/boot/grub" \
>> +			-r "hd0,msdos1" \
>> +			"/dev/$diskdev" \
>> +		&& touch /boot/grub/upgraded
> 
> This looks like almost same code used in two places, probably could be
> refactored in some common function used in both places?

This happens in different realm (on boot) and should be deleted in the future as
mentioned earlier. It does the same but adding that to common function used only on
one target, which will be used in single place (after preinit scrip removal) is
meaningless.

> 
>> diff --git a/target/linux/x86/base-files/lib/upgrade/platform.sh b/target/linux/x86/base-files/lib/upgrade/platform.sh
>> index 1a42fd3a11..05bbd97f3b 100644
>> --- a/target/linux/x86/base-files/lib/upgrade/platform.sh
>> +++ b/target/linux/x86/base-files/lib/upgrade/platform.sh
>> @@ -106,7 +106,8 @@ platform_do_upgrade() {
>>  			-m "/tmp/device.map" \
>>  			-d "/tmp/boot/boot/grub" \
>>  			-r "hd0,msdos1" \
>> -			"/dev/$diskdev"
>> +			"/dev/$diskdev" \
>> +		&& touch /boot/grub/upgraded
> 
> This probably should be in your other patch?

The patches are split up so that if the #3 is rejected there is no useless
"upgraded" file written to disk. The bootloader should be upgraded unconditionally
on every sysupgrade when all the "dangerous things" happen. The "upgraded" file is
only for preninit script in patch #3 to asses if sysupgrade or preinit script did
it's job.

> 
>> Currently bootloader always stays on the same version as when first
>> written to boot medium. That creates inconveniences as it always stays
>> with same features or/and bugs. Users wishing to add support to
>> additional modules or new version, would need to write the whole image,
>> potentially destroying previous system configuration. To fix these, this
>> commit adds additional routine to sysupgrade which upgrades
>> unconditionally the bootloader to the latest state provided by OpenWrt.
> 
> [...]
> 
>> +	#upgrade bootloader
> 
> This kind of comments are quite useless, I would rather use properly named
> function for self documenting code, like:
> 
>  if export_partdevice bootpart 1; then
>      upgrade_bootloader
>  fi

Ok, will move to separate function.

> 
>> +	if export_partdevice bootpart 1; then
>> +		mkdir -p /tmp/boot
>> +		mount -o rw,noatime "/dev/$bootpart" /tmp/boot
>> +		echo "(hd0) /dev/$diskdev" > /tmp/device.map
>> +
>> +		echo "Upgrading bootloader on /dev/$diskdev..."
>> +		grub-bios-setup \
>> +			-m "/tmp/device.map" \
>> +			-d "/tmp/boot/boot/grub" \
>> +			-r "hd0,msdos1" \
>> +			"/dev/$diskdev"
>> +
>> +		umount /tmp/boot
>> +	fi
> 
> This looks similar to the above, could be probably shared.
> 
> -- ynezz
> 

Regards

-- 
TMN

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel


More information about the openwrt-devel mailing list