| Message ID | 20190424191439.32298-2-klaus.kudielka@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Petr Štetiar |
| Headers | show |
| Series | Fix sysupgrade on Turris Omnia | expand |
W dniu 24.04.2019 o 21:14, Klaus Kudielka pisze: > Recently, upgrade device autodetection has been added to the mvebu target. > This exposes some shortcomings of the generic export_bootdevice function, > e.g. on the Turris Omnia: export_bootdevice silently reports the root > partition to be the boot device. This makes the sysupgrade process > fail at several places. > > Fix this by clearly distinguishing between /proc/cmdline arguments which > specify the boot disk, and those which specify the root partition. Only > in the latter case, strip off the partition, and do it consistently. > Include /dev/mmcblk* and /dev/sd* as potential arguments to "root=". > > Remove workarounds for the old, inconsistent behaviour from the following > targets: apm821xx, brcm2708, omap, sunxi. > > Fixes: 4e8345ff68 ("mvebu: base-files: autodetect upgrade device") > > Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com> Acked-by: Tomasz Maciej Nowak <tomek_n@o2.pl> Thanks. > --- > .../base-files/files/lib/upgrade/common.sh | 27 +++++++++++++------ > .../apm821xx/base-files/lib/upgrade/wdbook.sh | 11 +++----- > .../base-files/lib/upgrade/platform.sh | 11 +++----- > .../base-files/lib/preinit/79_move_config | 2 +- > .../omap/base-files/lib/upgrade/platform.sh | 7 +++-- > .../base-files/lib/preinit/79_move_config | 2 +- > .../sunxi/base-files/lib/upgrade/platform.sh | 7 +++-- > 7 files changed, 33 insertions(+), 34 deletions(-) > > diff --git a/package/base-files/files/lib/upgrade/common.sh b/package/base-files/files/lib/upgrade/common.sh > index b3a29fb32e..d906023dab 100644 > --- a/package/base-files/files/lib/upgrade/common.sh > +++ b/package/base-files/files/lib/upgrade/common.sh > @@ -101,24 +101,30 @@ get_magic_long() { > } > > export_bootdevice() { > - local cmdline uuid disk uevent line > + local cmdline bootdisk rootpart uuid disk uevent line > local MAJOR MINOR DEVNAME DEVTYPE > > if read cmdline < /proc/cmdline; then > case "$cmdline" in > *block2mtd=*) > - disk="${cmdline##*block2mtd=}" > - disk="${disk%%,*}" > + bootdisk="${cmdline##*block2mtd=}" > + bootdisk="${bootdisk%%,*}" > ;; > *root=*) > - disk="${cmdline##*root=}" > - disk="${disk%% *}" > + rootpart="${cmdline##*root=}" > + rootpart="${rootpart%% *}" > ;; > esac > > - case "$disk" in > + case "$bootdisk" in > + /dev/*) > + uevent="/sys/class/block/${bootdisk##*/}/uevent" > + ;; > + esac > + > + case "$rootpart" in > PARTUUID=[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-02) > - uuid="${disk#PARTUUID=}" > + uuid="${rootpart#PARTUUID=}" > uuid="${uuid%-02}" > for disk in $(find /dev -type b); do > set -- $(dd if=$disk bs=1 skip=440 count=4 2>/dev/null | hexdump -v -e '4/1 "%02x "') > @@ -128,7 +134,12 @@ export_bootdevice() { > fi > done > ;; > - /dev/*) > + /dev/mmcblk[0-9]p2) > + disk="${rootpart%p2}" > + uevent="/sys/class/block/${disk##*/}/uevent" > + ;; > + /dev/sd[a-z]2) > + disk="${rootpart%2}" > uevent="/sys/class/block/${disk##*/}/uevent" > ;; > esac > diff --git a/target/linux/apm821xx/base-files/lib/upgrade/wdbook.sh b/target/linux/apm821xx/base-files/lib/upgrade/wdbook.sh > index 2287e0619d..c191271149 100644 > --- a/target/linux/apm821xx/base-files/lib/upgrade/wdbook.sh > +++ b/target/linux/apm821xx/base-files/lib/upgrade/wdbook.sh > @@ -7,7 +7,7 @@ mbl_do_platform_check() { > > [ "$#" -gt 1 ] && return 1 > > - export_bootdevice && export_partdevice diskdev -2 || { > + export_bootdevice && export_partdevice diskdev 0 || { > echo "Unable to determine upgrade device" > return 1 > } > @@ -36,7 +36,7 @@ mbl_do_platform_check() { > mbl_do_upgrade() { > local diskdev partdev diff > > - export_bootdevice && export_partdevice diskdev -2 || { > + export_bootdevice && export_partdevice diskdev 0 || { > echo "Unable to determine upgrade device" > return 1 > } > @@ -70,10 +70,6 @@ mbl_do_upgrade() { > > #iterate over each partition from the image and write it to the boot disk > while read part start size; do > - # root is /dev/sd[a|b]2 and not /dev/sd[a|b] this causes some problem > - # one of which is this offset, I'm not sure what's the best fix, so > - # here's a WA. > - let part=$((part - 2)) > if export_partdevice partdev $part; then > echo "Writing image to /dev/$partdev..." > get_image "$@" | dd of="/dev/$partdev" ibs="512" obs=1M skip="$start" count="$size" conv=fsync > @@ -90,8 +86,7 @@ mbl_do_upgrade() { > mbl_copy_config() { > local partdev > > - # Same as above /dev/sd[a|b]2 is root, so /boot is -1 > - if export_partdevice partdev -1; then > + if export_partdevice partdev 1; then > mount -t ext4 -o rw,noatime "/dev/$partdev" /mnt > cp -af "$CONF_TAR" /mnt/ > umount /mnt > diff --git a/target/linux/brcm2708/base-files/lib/upgrade/platform.sh b/target/linux/brcm2708/base-files/lib/upgrade/platform.sh > index 62eede53d3..37e479272b 100644 > --- a/target/linux/brcm2708/base-files/lib/upgrade/platform.sh > +++ b/target/linux/brcm2708/base-files/lib/upgrade/platform.sh > @@ -9,7 +9,7 @@ platform_check_image() { > > [ "$#" -gt 1 ] && return 1 > > - export_bootdevice && export_partdevice diskdev -2 || { > + export_bootdevice && export_partdevice diskdev 0 || { > echo "Unable to determine upgrade device" > return 1 > } > @@ -38,7 +38,7 @@ platform_check_image() { > platform_do_upgrade() { > local diskdev partdev diff > > - export_bootdevice && export_partdevice diskdev -2 || { > + export_bootdevice && export_partdevice diskdev 0 || { > echo "Unable to determine upgrade device" > return 1 > } > @@ -72,10 +72,6 @@ platform_do_upgrade() { > > #iterate over each partition from the image and write it to the boot disk > while read part start size; do > - # root is /dev/sd[a|b]2 and not /dev/sd[a|b] this causes some problem > - # one of which is this offset, I'm not sure what's the best fix, so > - # here's a WA. > - let part=$((part - 2)) > if export_partdevice partdev $part; then > echo "Writing image to /dev/$partdev..." > get_image "$@" | dd of="/dev/$partdev" ibs="512" obs=1M skip="$start" count="$size" conv=fsync > @@ -92,8 +88,7 @@ platform_do_upgrade() { > platform_copy_config() { > local partdev > > - # Same as above /dev/sd[a|b]2 is root, so /boot is -1 > - if export_partdevice partdev -1; then > + if export_partdevice partdev 1; then > mkdir -p /boot > [ -f /boot/kernel.img ] || mount -t vfat -o rw,noatime "/dev/$partdev" /boot > cp -af "$CONF_TAR" /boot/ > diff --git a/target/linux/omap/base-files/lib/preinit/79_move_config b/target/linux/omap/base-files/lib/preinit/79_move_config > index c112588689..83171b3ba9 100644 > --- a/target/linux/omap/base-files/lib/preinit/79_move_config > +++ b/target/linux/omap/base-files/lib/preinit/79_move_config > @@ -6,7 +6,7 @@ move_config() { > > . /lib/upgrade/common.sh > > - if export_bootdevice && export_partdevice partdev -1; then > + if export_bootdevice && export_partdevice partdev 1; then > if mount -t vfat -o rw,noatime "/dev/$partdev" /mnt; then > if [ -f /mnt/sysupgrade.tgz ]; then > mv -f /mnt/sysupgrade.tgz / > diff --git a/target/linux/omap/base-files/lib/upgrade/platform.sh b/target/linux/omap/base-files/lib/upgrade/platform.sh > index 88ef4790e9..abe910b154 100644 > --- a/target/linux/omap/base-files/lib/upgrade/platform.sh > +++ b/target/linux/omap/base-files/lib/upgrade/platform.sh > @@ -1,7 +1,7 @@ > platform_check_image() { > local diskdev partdev diff > > - export_bootdevice && export_partdevice diskdev -2 || { > + export_bootdevice && export_partdevice diskdev 0 || { > echo "Unable to determine upgrade device" > return 1 > } > @@ -28,7 +28,7 @@ platform_check_image() { > platform_copy_config() { > local partdev > > - if export_partdevice partdev -1; then > + if export_partdevice partdev 1; then > mount -t vfat -o rw,noatime "/dev/$partdev" /mnt > cp -af "$CONF_TAR" /mnt/ > umount /mnt > @@ -38,7 +38,7 @@ platform_copy_config() { > platform_do_upgrade() { > local diskdev partdev diff > > - export_bootdevice && export_partdevice diskdev -2 || { > + export_bootdevice && export_partdevice diskdev 0 || { > echo "Unable to determine upgrade device" > return 1 > } > @@ -74,7 +74,6 @@ platform_do_upgrade() { > get_image "$@" | dd of="$diskdev" bs=1024 skip=8 seek=8 count=1016 conv=fsync > #iterate over each partition from the image and write it to the boot disk > while read part start size; do > - part="$(($part - 2))" > if export_partdevice partdev $part; then > echo "Writing image to /dev/$partdev..." > get_image "$@" | dd of="/dev/$partdev" ibs="512" obs=1M skip="$start" count="$size" conv=fsync > diff --git a/target/linux/sunxi/base-files/lib/preinit/79_move_config b/target/linux/sunxi/base-files/lib/preinit/79_move_config > index c112588689..83171b3ba9 100644 > --- a/target/linux/sunxi/base-files/lib/preinit/79_move_config > +++ b/target/linux/sunxi/base-files/lib/preinit/79_move_config > @@ -6,7 +6,7 @@ move_config() { > > . /lib/upgrade/common.sh > > - if export_bootdevice && export_partdevice partdev -1; then > + if export_bootdevice && export_partdevice partdev 1; then > if mount -t vfat -o rw,noatime "/dev/$partdev" /mnt; then > if [ -f /mnt/sysupgrade.tgz ]; then > mv -f /mnt/sysupgrade.tgz / > diff --git a/target/linux/sunxi/base-files/lib/upgrade/platform.sh b/target/linux/sunxi/base-files/lib/upgrade/platform.sh > index 88ef4790e9..abe910b154 100644 > --- a/target/linux/sunxi/base-files/lib/upgrade/platform.sh > +++ b/target/linux/sunxi/base-files/lib/upgrade/platform.sh > @@ -1,7 +1,7 @@ > platform_check_image() { > local diskdev partdev diff > > - export_bootdevice && export_partdevice diskdev -2 || { > + export_bootdevice && export_partdevice diskdev 0 || { > echo "Unable to determine upgrade device" > return 1 > } > @@ -28,7 +28,7 @@ platform_check_image() { > platform_copy_config() { > local partdev > > - if export_partdevice partdev -1; then > + if export_partdevice partdev 1; then > mount -t vfat -o rw,noatime "/dev/$partdev" /mnt > cp -af "$CONF_TAR" /mnt/ > umount /mnt > @@ -38,7 +38,7 @@ platform_copy_config() { > platform_do_upgrade() { > local diskdev partdev diff > > - export_bootdevice && export_partdevice diskdev -2 || { > + export_bootdevice && export_partdevice diskdev 0 || { > echo "Unable to determine upgrade device" > return 1 > } > @@ -74,7 +74,6 @@ platform_do_upgrade() { > get_image "$@" | dd of="$diskdev" bs=1024 skip=8 seek=8 count=1016 conv=fsync > #iterate over each partition from the image and write it to the boot disk > while read part start size; do > - part="$(($part - 2))" > if export_partdevice partdev $part; then > echo "Writing image to /dev/$partdev..." > get_image "$@" | dd of="/dev/$partdev" ibs="512" obs=1M skip="$start" count="$size" conv=fsync >
Klaus Kudielka <klaus.kudielka@gmail.com> [2019-04-24 21:14:38]: Hi Klaus, > Remove workarounds for the old, inconsistent behaviour from the following > targets: apm821xx, brcm2708, omap, sunxi. I'm willing to push the fix for mvebu (which the patch series is about), so please don't touch other targets. This should go into separate patch(es) and in order to push it, I would need some Tested-by, Reviewed-by or Acked-by for every platform separately. > Fixes: 4e8345ff68 ("mvebu: base-files: autodetect upgrade device") > > Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com> > Acked-by: Tomasz Maciej Nowak <tomek_n@o2.pl> Tomasz, you seem pretty confident about this changes, how is that? :-) -- ynezz
On 03.05.19 19:05, Petr Štetiar wrote: > Klaus Kudielka <klaus.kudielka@gmail.com> [2019-04-24 21:14:38]: > > Hi Klaus, > >> Remove workarounds for the old, inconsistent behaviour from the following >> targets: apm821xx, brcm2708, omap, sunxi. > I'm willing to push the fix for mvebu (which the patch series is about), so > please don't touch other targets. This should go into separate patch(es) and > in order to push it, I would need some Tested-by, Reviewed-by or Acked-by for > every platform separately. Petr, do I understand correctly, I should split this single patch into the common.sh one, and four additional ones (one per target)? -- Let me remind you that the common one *alone* breaks sysupgrade for those four targets, as Tomasz already pointed out earlier. In more detail: The root of the problem is that the *existing* export_bootdevice in /lib/upgrade/common.sh behaves differently, if the kernel is booted with root=/dev/..., or if it is booted with root=PARTUUID=... In the former case, it reports back major/minor of the root partition, in the latter case it reports back major/minor of the complete boot disk. The targets mentioned above have added workarounds to this behaviour, by specifying *negative* increments to the export_partdevice function. And then came the mvebu target to use export_bootdevice / export_partdevice as well. Now, different subtargets boot differently, and the workaround would be even more complex. I think now is the time to make export_bootdevice behave consistently, and to report major/minor of the boot disk, period. Consequently, those targets, which boot with root=/dev/... *and* use export_bootdevice / export_partdevice, have to be adapted to use positive increments, otherwise they are broken by the change to export_bootdevice. The targets affected were easy to spot with find & grep. > >> Fixes: 4e8345ff68 ("mvebu: base-files: autodetect upgrade device") >> >> Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com> >> Acked-by: Tomasz Maciej Nowak <tomek_n@o2.pl> > Tomasz, you seem pretty confident about this changes, how is that? :-) > > -- ynezz
Klaus Kudielka <klaus.kudielka@gmail.com> [2019-05-03 20:16:39]: > Let me remind you that the common one *alone* breaks sysupgrade for those > four targets, as Tomasz already pointed out earlier. Well, how could I know what was wrong with v1 if you didn't included the changes between v1 -> v2 in your v2 patch :-) Anyway, thanks for the explanation, it wasn't that much clear to me from the commit message, so if you don't mind, I'll include the details there as well in order to help it better understand to other folks. Merged into my staging tree https://git.openwrt.org/?p=openwrt/staging/ynezz.git;a=commit;h=195178f88ee7b3815f9bea66a2454ccfdf2135a5 > In more detail: > > The root of the problem is that the *existing* export_bootdevice in > /lib/upgrade/common.sh behaves differently, if the kernel is booted with > root=/dev/..., or if it is booted with root=PARTUUID=... > > In the former case, it reports back major/minor of the root partition, > in the latter case it reports back major/minor of the complete boot disk. > > The targets mentioned above have added workarounds to this behaviour, by > specifying *negative* increments to the export_partdevice function. > > And then came the mvebu target to use export_bootdevice / > export_partdevice as well. Now, different subtargets boot differently, > and the workaround would be even more complex. > > I think now is the time to make export_bootdevice behave consistently, > and to report major/minor of the boot disk, period. > > Consequently, those targets, which boot with root=/dev/... *and* use > export_bootdevice / export_partdevice, have to be adapted to use > positive increments, otherwise they are broken by the change > to export_bootdevice. > > The targets affected were easy to spot with find & grep.
On Friday, May 3, 2019 9:38:46 PM CEST Petr Štetiar wrote: > Klaus Kudielka <klaus.kudielka@gmail.com> [2019-05-03 20:16:39]: > > > Let me remind you that the common one *alone* breaks sysupgrade for those > > four targets, as Tomasz already pointed out earlier. > > Well, how could I know what was wrong with v1 if you didn't included the > changes between v1 -> v2 in your v2 patch :-) > > Anyway, thanks for the explanation, it wasn't that much clear to me from the > commit message, so if you don't mind, I'll include the details there as well > in order to help it better understand to other folks. > > Merged into my staging tree https://git.openwrt.org/?p=openwrt/staging/ynezz.git;a=commit;h=195178f88ee7b3815f9bea66a2454ccfdf2135a5 > > > In more detail: > > > > The root of the problem is that the *existing* export_bootdevice in > > /lib/upgrade/common.sh behaves differently, if the kernel is booted with > > root=/dev/..., or if it is booted with root=PARTUUID=... > > > > In the former case, it reports back major/minor of the root partition, > > in the latter case it reports back major/minor of the complete boot disk. > > > > The targets mentioned above have added workarounds to this behaviour, by > > specifying *negative* increments to the export_partdevice function. > > > > And then came the mvebu target to use export_bootdevice / > > export_partdevice as well. Now, different subtargets boot differently, > > and the workaround would be even more complex. > > > > I think now is the time to make export_bootdevice behave consistently, > > and to report major/minor of the boot disk, period. Just a note here: The export_bootdevice with it's PARTUUID-02 / sd[a-z]2 handling is not that great. Ideally the fixed partition should be avoided altogether in favour of a unique filesystem label or (less ideal) a filesystem UUID. Trouble is that squashfs does not support either. So that's where the fixed PARTUUID and sdX/mmcX device names come into play because otherwise the devices wouldn't boot. Sadly I think changes like this will probably go on until either squashfs gets these metadata image features or something replaces squashfs that has it. > > > > Consequently, those targets, which boot with root=/dev/... *and* use > > export_bootdevice / export_partdevice, have to be adapted to use > > positive increments, otherwise they are broken by the change > > to export_bootdevice. > > > > The targets affected were easy to spot with find & grep. True, it would have been great if the commit message included that export_bootdevice now consistenly works on those devices when the root= in the cmdline matches that PARTUUID-02, sd[a-z]2 or mmcblk[0-9]p2 and nothing else. Because there are still a few devices (I think Gemini DIR-685, DIR-313 and SQ201, and a Kirkwood GoFlex Home) that have the root= on sda1 or sda4 and could be converted to use the export_bootdevice for sysupgrade. But as of yet, I don't see that any of these devices have sysupgrade support. So your proposed patch is fine, unless you want to come up with a solution that can deal with the odd-balls.. Because that would be awesome! Cheers, Christian
On 04.05.19 16:51, Christian Lamparter wrote: > On Friday, May 3, 2019 9:38:46 PM CEST Petr Štetiar wrote: >> Klaus Kudielka <klaus.kudielka@gmail.com> [2019-05-03 20:16:39]: >> >>> Let me remind you that the common one *alone* breaks sysupgrade for those >>> four targets, as Tomasz already pointed out earlier. >> Well, how could I know what was wrong with v1 if you didn't included the >> changes between v1 -> v2 in your v2 patch :-) >> >> Anyway, thanks for the explanation, it wasn't that much clear to me from the >> commit message, so if you don't mind, I'll include the details there as well >> in order to help it better understand to other folks. >> >> Merged into my staging tree https://git.openwrt.org/?p=openwrt/staging/ynezz.git;a=commit;h=195178f88ee7b3815f9bea66a2454ccfdf2135a5 >> >>> In more detail: >>> >>> The root of the problem is that the *existing* export_bootdevice in >>> /lib/upgrade/common.sh behaves differently, if the kernel is booted with >>> root=/dev/..., or if it is booted with root=PARTUUID=... >>> >>> In the former case, it reports back major/minor of the root partition, >>> in the latter case it reports back major/minor of the complete boot disk. >>> >>> The targets mentioned above have added workarounds to this behaviour, by >>> specifying *negative* increments to the export_partdevice function. >>> >>> And then came the mvebu target to use export_bootdevice / >>> export_partdevice as well. Now, different subtargets boot differently, >>> and the workaround would be even more complex. >>> >>> I think now is the time to make export_bootdevice behave consistently, >>> and to report major/minor of the boot disk, period. > Just a note here: > > The export_bootdevice with it's PARTUUID-02 / sd[a-z]2 handling is not that > great. Ideally the fixed partition should be avoided altogether in favour of > a unique filesystem label or (less ideal) a filesystem UUID. > > Trouble is that squashfs does not support either. So that's where the fixed > PARTUUID and sdX/mmcX device names come into play because otherwise the devices > wouldn't boot. Sadly I think changes like this will probably go on until > either squashfs gets these metadata image features or something replaces > squashfs that has it. > >>> Consequently, those targets, which boot with root=/dev/... *and* use >>> export_bootdevice / export_partdevice, have to be adapted to use >>> positive increments, otherwise they are broken by the change >>> to export_bootdevice. >>> >>> The targets affected were easy to spot with find & grep. > True, it would have been great if the commit message included that > export_bootdevice now consistenly works on those devices when the > root= in the cmdline matches that PARTUUID-02, sd[a-z]2 or mmcblk[0-9]p2 > and nothing else. > > Because there are still a few devices (I think Gemini DIR-685, DIR-313 > and SQ201, and a Kirkwood GoFlex Home) that have the root= on sda1 or > sda4 and could be converted to use the export_bootdevice for sysupgrade. > > But as of yet, I don't see that any of these devices have sysupgrade support. > So your proposed patch is fine, unless you want to come up with a solution > that can deal with the odd-balls.. Because that would be awesome! Well, the primary goal of this patch was (and still is) to fix sysupgrade for Turris Omnia, without inventing yet another workaround for the rather schizophrenic behaviour. Personally I would prepare for potential future use cases in a separate patch. To deal with non-standard partition layouts, it could be as simple as replacing the trailing "2" with "[1-4]" in the six patterns of the $rootpart case statement... if this is what you mean? Regards, Klaus
On Sunday, May 5, 2019 10:11:40 AM CEST Klaus Kudielka wrote: > On 04.05.19 16:51, Christian Lamparter wrote: > > On Friday, May 3, 2019 9:38:46 PM CEST Petr Štetiar wrote: > >> Klaus Kudielka <klaus.kudielka@gmail.com> [2019-05-03 20:16:39]: > >> > >>> Let me remind you that the common one *alone* breaks sysupgrade for those > >>> four targets, as Tomasz already pointed out earlier. > >> Well, how could I know what was wrong with v1 if you didn't included the > >> changes between v1 -> v2 in your v2 patch :-) > >> > >> Anyway, thanks for the explanation, it wasn't that much clear to me from the > >> commit message, so if you don't mind, I'll include the details there as well > >> in order to help it better understand to other folks. > >> > >> Merged into my staging tree https://git.openwrt.org/?p=openwrt/staging/ynezz.git;a=commit;h=195178f88ee7b3815f9bea66a2454ccfdf2135a5 > >> > >>> In more detail: > >>> > >>> The root of the problem is that the *existing* export_bootdevice in > >>> /lib/upgrade/common.sh behaves differently, if the kernel is booted with > >>> root=/dev/..., or if it is booted with root=PARTUUID=... > >>> > >>> In the former case, it reports back major/minor of the root partition, > >>> in the latter case it reports back major/minor of the complete boot disk. > >>> > >>> The targets mentioned above have added workarounds to this behaviour, by > >>> specifying *negative* increments to the export_partdevice function. > >>> > >>> And then came the mvebu target to use export_bootdevice / > >>> export_partdevice as well. Now, different subtargets boot differently, > >>> and the workaround would be even more complex. > >>> > >>> I think now is the time to make export_bootdevice behave consistently, > >>> and to report major/minor of the boot disk, period. > > Just a note here: > > > > The export_bootdevice with it's PARTUUID-02 / sd[a-z]2 handling is not that > > great. Ideally the fixed partition should be avoided altogether in favour of > > a unique filesystem label or (less ideal) a filesystem UUID. > > > > Trouble is that squashfs does not support either. So that's where the fixed > > PARTUUID and sdX/mmcX device names come into play because otherwise the devices > > wouldn't boot. Sadly I think changes like this will probably go on until > > either squashfs gets these metadata image features or something replaces > > squashfs that has it. > > > >>> Consequently, those targets, which boot with root=/dev/... *and* use > >>> export_bootdevice / export_partdevice, have to be adapted to use > >>> positive increments, otherwise they are broken by the change > >>> to export_bootdevice. > >>> > >>> The targets affected were easy to spot with find & grep. > > True, it would have been great if the commit message included that > > export_bootdevice now consistenly works on those devices when the > > root= in the cmdline matches that PARTUUID-02, sd[a-z]2 or mmcblk[0-9]p2 > > and nothing else. > > > > Because there are still a few devices (I think Gemini DIR-685, DIR-313 > > and SQ201, and a Kirkwood GoFlex Home) that have the root= on sda1 or > > sda4 and could be converted to use the export_bootdevice for sysupgrade. > > > > But as of yet, I don't see that any of these devices have sysupgrade support. > > So your proposed patch is fine, unless you want to come up with a solution > > that can deal with the odd-balls.. Because that would be awesome! > Well, the primary goal of this patch was (and still is) to fix sysupgrade > for Turris Omnia, without inventing yet another workaround for the rather > schizophrenic behaviour. Well, from afar it really looks like your patch replaces *common*.sh code that works for every *generic* cmdline root=/dev/* with something that only considers root=/dev/mmcblk[0-9]p2|/dev/sd[a-z]2 . Don't you see why this a point of contention? > Personally I would prepare for potential future use cases in a separate > patch. To deal with non-standard partition layouts, it could be as simple > as replacing the trailing "2" with "[1-4]" in the six patterns of the > $rootpart case statement... if this is what you mean? I think it's very device specificy to say that the second partition is the root partition. Ideally, the a uuid or fs label should be used as root=. But in case of squashfs it's not possible, only PARTUUID or device file name works. Cheers, Christian
diff --git a/package/base-files/files/lib/upgrade/common.sh b/package/base-files/files/lib/upgrade/common.sh index b3a29fb32e..d906023dab 100644 --- a/package/base-files/files/lib/upgrade/common.sh +++ b/package/base-files/files/lib/upgrade/common.sh @@ -101,24 +101,30 @@ get_magic_long() { } export_bootdevice() { - local cmdline uuid disk uevent line + local cmdline bootdisk rootpart uuid disk uevent line local MAJOR MINOR DEVNAME DEVTYPE if read cmdline < /proc/cmdline; then case "$cmdline" in *block2mtd=*) - disk="${cmdline##*block2mtd=}" - disk="${disk%%,*}" + bootdisk="${cmdline##*block2mtd=}" + bootdisk="${bootdisk%%,*}" ;; *root=*) - disk="${cmdline##*root=}" - disk="${disk%% *}" + rootpart="${cmdline##*root=}" + rootpart="${rootpart%% *}" ;; esac - case "$disk" in + case "$bootdisk" in + /dev/*) + uevent="/sys/class/block/${bootdisk##*/}/uevent" + ;; + esac + + case "$rootpart" in PARTUUID=[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-02) - uuid="${disk#PARTUUID=}" + uuid="${rootpart#PARTUUID=}" uuid="${uuid%-02}" for disk in $(find /dev -type b); do set -- $(dd if=$disk bs=1 skip=440 count=4 2>/dev/null | hexdump -v -e '4/1 "%02x "') @@ -128,7 +134,12 @@ export_bootdevice() { fi done ;; - /dev/*) + /dev/mmcblk[0-9]p2) + disk="${rootpart%p2}" + uevent="/sys/class/block/${disk##*/}/uevent" + ;; + /dev/sd[a-z]2) + disk="${rootpart%2}" uevent="/sys/class/block/${disk##*/}/uevent" ;; esac diff --git a/target/linux/apm821xx/base-files/lib/upgrade/wdbook.sh b/target/linux/apm821xx/base-files/lib/upgrade/wdbook.sh index 2287e0619d..c191271149 100644 --- a/target/linux/apm821xx/base-files/lib/upgrade/wdbook.sh +++ b/target/linux/apm821xx/base-files/lib/upgrade/wdbook.sh @@ -7,7 +7,7 @@ mbl_do_platform_check() { [ "$#" -gt 1 ] && return 1 - export_bootdevice && export_partdevice diskdev -2 || { + export_bootdevice && export_partdevice diskdev 0 || { echo "Unable to determine upgrade device" return 1 } @@ -36,7 +36,7 @@ mbl_do_platform_check() { mbl_do_upgrade() { local diskdev partdev diff - export_bootdevice && export_partdevice diskdev -2 || { + export_bootdevice && export_partdevice diskdev 0 || { echo "Unable to determine upgrade device" return 1 } @@ -70,10 +70,6 @@ mbl_do_upgrade() { #iterate over each partition from the image and write it to the boot disk while read part start size; do - # root is /dev/sd[a|b]2 and not /dev/sd[a|b] this causes some problem - # one of which is this offset, I'm not sure what's the best fix, so - # here's a WA. - let part=$((part - 2)) if export_partdevice partdev $part; then echo "Writing image to /dev/$partdev..." get_image "$@" | dd of="/dev/$partdev" ibs="512" obs=1M skip="$start" count="$size" conv=fsync @@ -90,8 +86,7 @@ mbl_do_upgrade() { mbl_copy_config() { local partdev - # Same as above /dev/sd[a|b]2 is root, so /boot is -1 - if export_partdevice partdev -1; then + if export_partdevice partdev 1; then mount -t ext4 -o rw,noatime "/dev/$partdev" /mnt cp -af "$CONF_TAR" /mnt/ umount /mnt diff --git a/target/linux/brcm2708/base-files/lib/upgrade/platform.sh b/target/linux/brcm2708/base-files/lib/upgrade/platform.sh index 62eede53d3..37e479272b 100644 --- a/target/linux/brcm2708/base-files/lib/upgrade/platform.sh +++ b/target/linux/brcm2708/base-files/lib/upgrade/platform.sh @@ -9,7 +9,7 @@ platform_check_image() { [ "$#" -gt 1 ] && return 1 - export_bootdevice && export_partdevice diskdev -2 || { + export_bootdevice && export_partdevice diskdev 0 || { echo "Unable to determine upgrade device" return 1 } @@ -38,7 +38,7 @@ platform_check_image() { platform_do_upgrade() { local diskdev partdev diff - export_bootdevice && export_partdevice diskdev -2 || { + export_bootdevice && export_partdevice diskdev 0 || { echo "Unable to determine upgrade device" return 1 } @@ -72,10 +72,6 @@ platform_do_upgrade() { #iterate over each partition from the image and write it to the boot disk while read part start size; do - # root is /dev/sd[a|b]2 and not /dev/sd[a|b] this causes some problem - # one of which is this offset, I'm not sure what's the best fix, so - # here's a WA. - let part=$((part - 2)) if export_partdevice partdev $part; then echo "Writing image to /dev/$partdev..." get_image "$@" | dd of="/dev/$partdev" ibs="512" obs=1M skip="$start" count="$size" conv=fsync @@ -92,8 +88,7 @@ platform_do_upgrade() { platform_copy_config() { local partdev - # Same as above /dev/sd[a|b]2 is root, so /boot is -1 - if export_partdevice partdev -1; then + if export_partdevice partdev 1; then mkdir -p /boot [ -f /boot/kernel.img ] || mount -t vfat -o rw,noatime "/dev/$partdev" /boot cp -af "$CONF_TAR" /boot/ diff --git a/target/linux/omap/base-files/lib/preinit/79_move_config b/target/linux/omap/base-files/lib/preinit/79_move_config index c112588689..83171b3ba9 100644 --- a/target/linux/omap/base-files/lib/preinit/79_move_config +++ b/target/linux/omap/base-files/lib/preinit/79_move_config @@ -6,7 +6,7 @@ move_config() { . /lib/upgrade/common.sh - if export_bootdevice && export_partdevice partdev -1; then + if export_bootdevice && export_partdevice partdev 1; then if mount -t vfat -o rw,noatime "/dev/$partdev" /mnt; then if [ -f /mnt/sysupgrade.tgz ]; then mv -f /mnt/sysupgrade.tgz / diff --git a/target/linux/omap/base-files/lib/upgrade/platform.sh b/target/linux/omap/base-files/lib/upgrade/platform.sh index 88ef4790e9..abe910b154 100644 --- a/target/linux/omap/base-files/lib/upgrade/platform.sh +++ b/target/linux/omap/base-files/lib/upgrade/platform.sh @@ -1,7 +1,7 @@ platform_check_image() { local diskdev partdev diff - export_bootdevice && export_partdevice diskdev -2 || { + export_bootdevice && export_partdevice diskdev 0 || { echo "Unable to determine upgrade device" return 1 } @@ -28,7 +28,7 @@ platform_check_image() { platform_copy_config() { local partdev - if export_partdevice partdev -1; then + if export_partdevice partdev 1; then mount -t vfat -o rw,noatime "/dev/$partdev" /mnt cp -af "$CONF_TAR" /mnt/ umount /mnt @@ -38,7 +38,7 @@ platform_copy_config() { platform_do_upgrade() { local diskdev partdev diff - export_bootdevice && export_partdevice diskdev -2 || { + export_bootdevice && export_partdevice diskdev 0 || { echo "Unable to determine upgrade device" return 1 } @@ -74,7 +74,6 @@ platform_do_upgrade() { get_image "$@" | dd of="$diskdev" bs=1024 skip=8 seek=8 count=1016 conv=fsync #iterate over each partition from the image and write it to the boot disk while read part start size; do - part="$(($part - 2))" if export_partdevice partdev $part; then echo "Writing image to /dev/$partdev..." get_image "$@" | dd of="/dev/$partdev" ibs="512" obs=1M skip="$start" count="$size" conv=fsync diff --git a/target/linux/sunxi/base-files/lib/preinit/79_move_config b/target/linux/sunxi/base-files/lib/preinit/79_move_config index c112588689..83171b3ba9 100644 --- a/target/linux/sunxi/base-files/lib/preinit/79_move_config +++ b/target/linux/sunxi/base-files/lib/preinit/79_move_config @@ -6,7 +6,7 @@ move_config() { . /lib/upgrade/common.sh - if export_bootdevice && export_partdevice partdev -1; then + if export_bootdevice && export_partdevice partdev 1; then if mount -t vfat -o rw,noatime "/dev/$partdev" /mnt; then if [ -f /mnt/sysupgrade.tgz ]; then mv -f /mnt/sysupgrade.tgz / diff --git a/target/linux/sunxi/base-files/lib/upgrade/platform.sh b/target/linux/sunxi/base-files/lib/upgrade/platform.sh index 88ef4790e9..abe910b154 100644 --- a/target/linux/sunxi/base-files/lib/upgrade/platform.sh +++ b/target/linux/sunxi/base-files/lib/upgrade/platform.sh @@ -1,7 +1,7 @@ platform_check_image() { local diskdev partdev diff - export_bootdevice && export_partdevice diskdev -2 || { + export_bootdevice && export_partdevice diskdev 0 || { echo "Unable to determine upgrade device" return 1 } @@ -28,7 +28,7 @@ platform_check_image() { platform_copy_config() { local partdev - if export_partdevice partdev -1; then + if export_partdevice partdev 1; then mount -t vfat -o rw,noatime "/dev/$partdev" /mnt cp -af "$CONF_TAR" /mnt/ umount /mnt @@ -38,7 +38,7 @@ platform_copy_config() { platform_do_upgrade() { local diskdev partdev diff - export_bootdevice && export_partdevice diskdev -2 || { + export_bootdevice && export_partdevice diskdev 0 || { echo "Unable to determine upgrade device" return 1 } @@ -74,7 +74,6 @@ platform_do_upgrade() { get_image "$@" | dd of="$diskdev" bs=1024 skip=8 seek=8 count=1016 conv=fsync #iterate over each partition from the image and write it to the boot disk while read part start size; do - part="$(($part - 2))" if export_partdevice partdev $part; then echo "Writing image to /dev/$partdev..." get_image "$@" | dd of="/dev/$partdev" ibs="512" obs=1M skip="$start" count="$size" conv=fsync
Recently, upgrade device autodetection has been added to the mvebu target. This exposes some shortcomings of the generic export_bootdevice function, e.g. on the Turris Omnia: export_bootdevice silently reports the root partition to be the boot device. This makes the sysupgrade process fail at several places. Fix this by clearly distinguishing between /proc/cmdline arguments which specify the boot disk, and those which specify the root partition. Only in the latter case, strip off the partition, and do it consistently. Include /dev/mmcblk* and /dev/sd* as potential arguments to "root=". Remove workarounds for the old, inconsistent behaviour from the following targets: apm821xx, brcm2708, omap, sunxi. Fixes: 4e8345ff68 ("mvebu: base-files: autodetect upgrade device") Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com> --- .../base-files/files/lib/upgrade/common.sh | 27 +++++++++++++------ .../apm821xx/base-files/lib/upgrade/wdbook.sh | 11 +++----- .../base-files/lib/upgrade/platform.sh | 11 +++----- .../base-files/lib/preinit/79_move_config | 2 +- .../omap/base-files/lib/upgrade/platform.sh | 7 +++-- .../base-files/lib/preinit/79_move_config | 2 +- .../sunxi/base-files/lib/upgrade/platform.sh | 7 +++-- 7 files changed, 33 insertions(+), 34 deletions(-)