Hi Petr.
W dniu 27.05.2019 o 18:17, Petr Štetiar pisze:
> Tomasz Maciej Nowak <tome...@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@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
