[OpenWrt-Devel,v2,1/2] base-files: improve lib/upgrade/common.sh
diff mbox series

Message ID 20190424191439.32298-2-klaus.kudielka@gmail.com
State Superseded, archived
Delegated to: Petr Štetiar
Headers show
Series
  • Fix sysupgrade on Turris Omnia
Related show

Commit Message

Klaus Kudielka April 24, 2019, 7:14 p.m. UTC
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(-)

Comments

Tomasz Maciej Nowak April 25, 2019, 12:39 p.m. UTC | #1
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
>
Petr Štetiar May 3, 2019, 5:05 p.m. UTC | #2
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
Klaus Kudielka May 3, 2019, 6:16 p.m. UTC | #3
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
Petr Štetiar May 3, 2019, 7:38 p.m. UTC | #4
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.
Christian Lamparter May 4, 2019, 2:51 p.m. UTC | #5
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
Klaus Kudielka May 5, 2019, 8:11 a.m. UTC | #6
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
Christian Lamparter May 5, 2019, 2:35 p.m. UTC | #7
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

Patch
diff mbox series

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