[OpenWrt-Devel] [PATCH][RFC] x86: Save and restore partition table during upgrade

[Jo-Philipp Wich]

Hi.

> This patch will save the partition table before an upgrade.
> If the installed image has not changed the partition structure
> the saved table will be restored, ensuring any user created
> partitions will be present after the upgrade.

That sounds odd. If I'd start to rely on such a feature I'd be really
surprised if my partition table suddenly was not backed up after an
upgrade.

Can you please elaborate some more on the intended use case?
Some other stylistic comments inline.

> An option is added to sysupgrade to disable this feature.
> 
> Signed-off-by: Rob Mosher <nyt-openwrt at countercultured.net>
> ---
>  package/base-files/files/lib/upgrade/common.sh     |  1 +
>  package/base-files/files/sbin/sysupgrade           |  3 ++
>  target/linux/x86/Makefile                          |  2 +
>  .../linux/x86/base-files/lib/upgrade/platform.sh   | 45 ++++++++++++++++++++++
>  4 files changed, 51 insertions(+)
> 
> diff --git a/package/base-files/files/lib/upgrade/common.sh b/package/base-files/files/lib/upgrade/common.sh
> index 761b4c1..aed7f8e 100644
> --- a/package/base-files/files/lib/upgrade/common.sh
> +++ b/package/base-files/files/lib/upgrade/common.sh
> @@ -67,6 +67,7 @@ run_ramfs() { # <command> [...]
>  	install_bin /usr/sbin/ubirsvol
>  	install_bin /usr/sbin/ubirmvol
>  	install_bin /usr/sbin/ubimkvol
> +	install_bin /usr/sbin/sfdisk
>  	for file in $RAMFS_COPY_BIN; do
>  		install_bin ${file//:/ }
>  	done
> diff --git a/package/base-files/files/sbin/sysupgrade b/package/base-files/files/sbin/sysupgrade
> index 93f0749..2f441f8 100755
> --- a/package/base-files/files/sbin/sysupgrade
> +++ b/package/base-files/files/sbin/sysupgrade
> @@ -10,6 +10,7 @@ export INTERACTIVE=0
>  export VERBOSE=1
>  export SAVE_CONFIG=1
>  export SAVE_OVERLAY=0
> +export SAVE_PARTITIONS=1
>  export DELAY=
>  export CONF_IMAGE=
>  export CONF_BACKUP_LIST=0
> @@ -29,6 +30,7 @@ while [ -n "$1" ]; do
>  		-q) export VERBOSE="$(($VERBOSE - 1))";;
>  		-n) export SAVE_CONFIG=0;;
>  		-c) export SAVE_OVERLAY=1;;
> +		-p) export SAVE_PARTITIONS=0;;
>  		-b|--create-backup) export CONF_BACKUP="$2" NEED_IMAGE=1; shift;;
>  		-r|--restore-backup) export CONF_RESTORE="$2" NEED_IMAGE=1; shift;;
>  		-l|--list-backup) export CONF_BACKUP_LIST=1; break;;
> @@ -62,6 +64,7 @@ upgrade-option:
>  	-i           interactive mode
>  	-c           attempt to preserve all changed files in /etc/
>  	-n           do not save configuration over reflash
> +	-p           do not attempt to restore the partition table after flash.
>  	-T | --test
>  	             Verify image and config .tar.gz but do not actually flash.
>  	-F | --force
> diff --git a/target/linux/x86/Makefile b/target/linux/x86/Makefile
> index e4bc0d9..e5b4378 100644
> --- a/target/linux/x86/Makefile
> +++ b/target/linux/x86/Makefile
> @@ -13,6 +13,8 @@ FEATURES:=squashfs ext4 vdi vmdk pcmcia targz
>  SUBTARGETS=generic xen_domu ep80579 geode kvm_guest 64
>  MAINTAINER:=Felix Fietkau <nbd at openwrt.org>
>  
> +DEFAULT_PACKAGES += sfdisk
> +
>  KERNEL_PATCHVER:=4.4
>  
>  KERNELNAME:=bzImage
> diff --git a/target/linux/x86/base-files/lib/upgrade/platform.sh b/target/linux/x86/base-files/lib/upgrade/platform.sh
> index 73ab5ef..7f5a2b1 100644
> --- a/target/linux/x86/base-files/lib/upgrade/platform.sh
> +++ b/target/linux/x86/base-files/lib/upgrade/platform.sh
> @@ -55,12 +55,57 @@ platform_copy_config() {
>  	fi
>  }
>  
> +save_bootparts() {
> +	disk=${BOOTPART%[0-9]}
> +	if [ -b $disk ]; then
> +		echo "Backing up partition table..."
> +		sfdisk -d $disk > /tmp/sfdisk.before
> +		grep size= /tmp/sfdisk.before | grep -E -v 'size=\s+0,' > /tmp/sfdisk.before.nonempty

Why two "grep" invocations here? Also always quote variables please.

> +	fi
> +}
> +
> +restore_bootparts() {
> +	disk=${BOOTPART%[0-9]}
> +	if [ -b $disk ]; then
> +		sfdisk -d $disk > /tmp/sfdisk.after
> +		grep size= /tmp/sfdisk.after | grep -E -v 'size=\s+0,' > /tmp/sfdisk.after.nonempty

Avoid the duplicate grep.

> +		before=$(cat /tmp/sfdisk.before.nonempty)
> +		after=$(cat /tmp/sfdisk.after.nonempty)

Always quote variables.

> +
> +		#ensure we have both partition tables

This comment is superfluous.

> +		if [ -z "$before" -o -z "$after" ]; then
> +			echo "Could not read partition table"
> +			return 1
> +		fi
> +		
> +		#if nothing changed, we do not need to restore
> +		if [ "$before" = "$after" ]; then
> +			echo "Parition layout unchanged"
> +			return 0
> +		fi
> +
> +		diff=$(grep -F -x -v -f /tmp/sfdisk.before.nonempty /tmp/sfdisk.after.nonempty)
> +
> +		#if partition layout changed, do not restore

This comment is superfluous.

> +		if [ -n "$diff" ]; then
> +			echo "Partition layout changed, not restoring."
> +			return 1
> +		fi
> +
> +		echo "Restoring partition table..."
> +		sfdisk $disk --force < /tmp/sfdisk.before >&/dev/null
> +	fi
> +}
> +
>  platform_do_upgrade() {
>  	platform_export_bootpart
>  
>  	if [ -b "${BOOTPART%[0-9]}" ]; then
>  		sync
> +		[ $SAVE_PARTITIONS = "1" ] && save_bootparts

You can probably move the enabled check into the procedures themselves.

>  		get_image "$@" | dd of="${BOOTPART%[0-9]}" bs=4096 conv=fsync
>  		sleep 1
> +		[ $SAVE_PARTITIONS = "1" ] && restore_bootparts
>  	fi
>  }
> +
> 

~ Jow

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20160117/248589bc/attachment.sig>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel