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

[Petr Štetiar]

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.

> +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?

> 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?

> 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

> +	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

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