Hi,

when looking at the merged patch (unfortunately only then), I found some 
"issues" (see below):

> -----Original Message-----
> From: openwrt-devel [mailto:[email protected]] On 
> Behalf Of Rafal Milecki
> Sent: Freitag, 6. September 2019 07:11
> To: [email protected]
> Cc: Rafał Miłecki <[email protected]>; Jonas Gorski <[email protected]>; 
> Jo-Philipp Wich <[email protected]>; John Crispin
> <[email protected]>
> Subject: [OpenWrt-Devel] [PATCH 3/3] treewide: sysupgrade: use 
> $UPGRADE_BACKUP to check for backup
> 
> From: Rafał Miłecki <[email protected]>
> 
> Now that $UPGRADE_BACKUP is set conditionally there is no need to check
> the $UPGRADE_OPT_SAVE_CONFIG anymore. All conditions can be simplified.
> 
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
>  package/base-files/files/lib/upgrade/common.sh          | 2 +-
>  package/base-files/files/lib/upgrade/do_stage2          | 2 +-
>  package/base-files/files/sbin/sysupgrade                | 1 -
>  target/linux/ar71xx/base-files/lib/upgrade/dir825.sh    | 2 +-
>  target/linux/ar71xx/base-files/lib/upgrade/openmesh.sh  | 2 +-
>  target/linux/ar71xx/base-files/lib/upgrade/platform.sh  | 4 ++--
>  target/linux/ath25/base-files/lib/upgrade/platform.sh   | 2 +-
>  target/linux/ath79/base-files/lib/upgrade/platform.sh   | 4 ++--
>  target/linux/imx6/base-files/lib/upgrade/platform.sh    | 2 +-
>  target/linux/ipq40xx/base-files/lib/upgrade/openmesh.sh | 2 +-
>  target/linux/ipq40xx/base-files/lib/upgrade/platform.sh | 2 +-
>  target/linux/ixp4xx/base-files/lib/upgrade/platform.sh  | 2 +-
>  12 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/package/base-files/files/lib/upgrade/common.sh 
> b/package/base-files/files/lib/upgrade/common.sh
> index 8e7866f698..0d3162d4fc 100644
> --- a/package/base-files/files/lib/upgrade/common.sh
> +++ b/package/base-files/files/lib/upgrade/common.sh
> @@ -220,7 +220,7 @@ indicate_upgrade() {
>  # $(2): (optional) pipe command to extract firmware, e.g. dd bs=n skip=m
>  default_do_upgrade() {
>       sync
> -     if [ "$UPGRADE_OPT_SAVE_CONFIG" -eq 1 ]; then
> +     if [ -n "$UPGRADE_BACKUP" ]; then

Any reason why this is "-n" and not "-f" as below?

>               get_image "$1" "$2" | mtd $MTD_ARGS $MTD_CONFIG_ARGS -j 
> "$UPGRADE_BACKUP" write - "${PART_NAME:-
> image}"
>       else
>               get_image "$1" "$2" | mtd $MTD_ARGS write - 
> "${PART_NAME:-image}"
> diff --git a/package/base-files/files/lib/upgrade/do_stage2 
> b/package/base-files/files/lib/upgrade/do_stage2
> index 0e6cc1bfc3..0e32445743 100755
> --- a/package/base-files/files/lib/upgrade/do_stage2
> +++ b/package/base-files/files/lib/upgrade/do_stage2
> @@ -11,7 +11,7 @@ else
>       default_do_upgrade "$IMAGE"
>  fi
> 
> -if [ "$UPGRADE_OPT_SAVE_CONFIG" -eq 1 ] && type 'platform_copy_config' 
> >/dev/null 2>/dev/null; then
> +if [ -n "$UPGRADE_BACKUP" ] && type 'platform_copy_config' >/dev/null 
> 2>/dev/null; then

Here I'm not so sure about "-f" vs. "-n" ...

>       platform_copy_config
>  fi
> 
> diff --git a/package/base-files/files/sbin/sysupgrade 
> b/package/base-files/files/sbin/sysupgrade
> index f18143bff4..935d08048e 100755
> --- a/package/base-files/files/sbin/sysupgrade
> +++ b/package/base-files/files/sbin/sysupgrade
> @@ -371,7 +371,6 @@ else
>               $backup_attr
>               \"command\": $(json_string "$COMMAND"),
>               \"options\": {
> -                     \"save_config\": $SAVE_CONFIG,
>                       \"save_partitions\": $SAVE_PARTITIONS
>               }
>       }"
> diff --git a/target/linux/ar71xx/base-files/lib/upgrade/dir825.sh 
> b/target/linux/ar71xx/base-files/lib/upgrade/dir825.sh
> index c694c2e6f2..e991a06b7a 100644
> --- a/target/linux/ar71xx/base-files/lib/upgrade/dir825.sh
> +++ b/target/linux/ar71xx/base-files/lib/upgrade/dir825.sh
> @@ -75,7 +75,7 @@ dir825b_do_upgrade_combined() {
> 
>       if [ -n "$fw_mtd" ] &&  [ ${fw_blocks:-0} -gt 0 ]; then
>               local append=""
> -             [ -f "$UPGRADE_BACKUP" -a "$UPGRADE_OPT_SAVE_CONFIG" -eq 1 ] && 
> append="-j $UPGRADE_BACKUP"
> +             [ -f "$UPGRADE_BACKUP" ] && append="-j $UPGRADE_BACKUP"
> 
>               sync
>               dd if="$fw_file" bs=64k skip=1 count=$fw_blocks 2>/dev/null | \
> diff --git a/target/linux/ar71xx/base-files/lib/upgrade/openmesh.sh 
> b/target/linux/ar71xx/base-files/lib/upgrade/openmesh.sh
> index 8536d4ba4a..f43bdcea7f 100644
> --- a/target/linux/ar71xx/base-files/lib/upgrade/openmesh.sh
> +++ b/target/linux/ar71xx/base-files/lib/upgrade/openmesh.sh
> @@ -159,7 +159,7 @@ platform_do_upgrade_openmesh()
>       local cfg_size= kernel_size= rootfs_size=
>       local append=""
> 
> -     [ -f "$UPGRADE_BACKUP" -a "$UPGRADE_OPT_SAVE_CONFIG" -eq 1 ] && 
> append="-j $UPGRADE_BACKUP"
> +     [ -f "$UPGRADE_BACKUP" ] && append="-j $UPGRADE_BACKUP"
> 
>       cfg_size=$(dd if="$img_path" bs=2 skip=35 count=4 2>/dev/null)
>       kernel_size=$(dd if="$img_path" bs=2 skip=71 count=4 2>/dev/null)
> diff --git a/target/linux/ar71xx/base-files/lib/upgrade/platform.sh 
> b/target/linux/ar71xx/base-files/lib/upgrade/platform.sh
> index 726163291d..86e7b6386b 100755
> --- a/target/linux/ar71xx/base-files/lib/upgrade/platform.sh
> +++ b/target/linux/ar71xx/base-files/lib/upgrade/platform.sh
> @@ -65,7 +65,7 @@ platform_do_upgrade_combined() {
>       then
>               local rootfspart=$(platform_find_rootfspart "$partitions" 
> "$kernelpart")
>               local append=""
> -             [ -f "$UPGRADE_BACKUP" -a "$UPGRADE_OPT_SAVE_CONFIG" -eq 1 ] && 
> append="-j $UPGRADE_BACKUP"
> +             [ -f "$UPGRADE_BACKUP" ] && append="-j $UPGRADE_BACKUP"
> 
>               if [ "$PLATFORM_DO_UPGRADE_COMBINED_SEPARATE_MTD" -ne 1 ]; then
>                   ( dd if="$1" bs=$CI_BLKSZ skip=1 count=$kern_blocks 
> 2>/dev/null; \
> @@ -164,7 +164,7 @@ platform_do_upgrade_compex() {
> 
>       if [ -n "$fw_mtd" ] &&  [ ${fw_blocks:-0} -gt 0 ]; then
>               local append=""
> -             [ -f "$UPGRADE_BACKUP" -a "$UPGRADE_OPT_SAVE_CONFIG" -eq 1 ] && 
> append="-j $UPGRADE_BACKUPs"
> +             [ -f "$UPGRADE_BACKUP" ] && append="-j $UPGRADE_BACKUPs"

Is there a reason for the trailing "s" here or is this a typo: ="-j 
$UPGRADE_BACKUPs" ?

> 
>               sync
>               dd if="$fw_file" bs=64k skip=1 count=$fw_blocks 2>/dev/null | \
> diff --git a/target/linux/ath25/base-files/lib/upgrade/platform.sh 
> b/target/linux/ath25/base-files/lib/upgrade/platform.sh
> index 0dde103605..778bbf5a39 100644
> --- a/target/linux/ath25/base-files/lib/upgrade/platform.sh
> +++ b/target/linux/ath25/base-files/lib/upgrade/platform.sh
> @@ -67,7 +67,7 @@ platform_do_upgrade() {
>          [ ${erase_size:-0} -gt 0 ];
>       then
>               local append=""
> -             [ -f "$UPGRADE_BACKUP" -a "$UPGRADE_OPT_SAVE_CONFIG" -eq 1 ] && 
> append="-j $UPGRADE_BACKUP"
> +             [ -f "$UPGRADE_BACKUP" ] && append="-j $UPGRADE_BACKUP"
> 
>               ( dd if="$1" bs=$CI_BLKSZ skip=1 count=$kern_blocks 
> 2>/dev/null; \
>                 dd if="$1" bs=$CI_BLKSZ skip=$((1+$kern_blocks)) 
> count=$root_blocks 2>/dev/null ) | \
> diff --git a/target/linux/ath79/base-files/lib/upgrade/platform.sh 
> b/target/linux/ath79/base-files/lib/upgrade/platform.sh
> index f7e62143e7..f4fca06384 100644
> --- a/target/linux/ath79/base-files/lib/upgrade/platform.sh
> +++ b/target/linux/ath79/base-files/lib/upgrade/platform.sh
> @@ -14,7 +14,7 @@ redboot_fis_do_upgrade() {
>       if [ "$magic" = "4349" ]; then
>               local kern_length=0x$(dd if="$sysup_file" bs=2 skip=1 count=4 
> 2>/dev/null)
> 
> -             [ -f "$UPGRADE_BACKUP" -a 
> "$UPGRADE_OPT_UPGRADE_OPT_SAVE_CONFIG" -eq 1 ] && append="-j
> $UPGRADE_BACKUP"
> +             [ -f "$UPGRADE_BACKUP" ] && append="-j $UPGRADE_BACKUP"
>               dd if="$sysup_file" bs=64k skip=1 2>/dev/null | \
>                       mtd -r $append 
> -F$kern_part:$kern_length:0x80060000,rootfs write - $kern_part:rootfs
> 
> @@ -22,7 +22,7 @@ redboot_fis_do_upgrade() {
>               local board_dir=$(tar tf $sysup_file | grep -m 1 
> '^sysupgrade-.*/$')
>               local kern_length=$(tar xf $sysup_file ${board_dir}kernel -O | 
> wc -c)
> 
> -             [ -f "$UPGRADE_BACKUP" -a 
> "$UPGRADE_OPT_UPGRADE_OPT_SAVE_CONFIG" -eq 1 ] && append="-j
> $UPGRADE_BACKUP"
> +             [ -f "$UPGRADE_BACKUP" ] && append="-j $UPGRADE_BACKUP"
>               tar xf $sysup_file ${board_dir}kernel ${board_dir}root -O | \
>                       mtd -r $append 
> -F$kern_part:$kern_length:0x80060000,rootfs write - $kern_part:rootfs
> 
> diff --git a/target/linux/imx6/base-files/lib/upgrade/platform.sh 
> b/target/linux/imx6/base-files/lib/upgrade/platform.sh
> index 9414b18710..a090cc080b 100755
> --- a/target/linux/imx6/base-files/lib/upgrade/platform.sh
> +++ b/target/linux/imx6/base-files/lib/upgrade/platform.sh
> @@ -75,7 +75,7 @@ platform_pre_upgrade() {
> 
>       case "$board" in
>       apalis*)
> -             [ "$UPGRADE_OPT_SAVE_CONFIG" -eq 0 ] && {
> +             [ -z "$UPGRADE_BACKUP" ] && {

Really "-z" or "! -f"?

>                       jffs2reset -y
>                       umount /overlay
>               }
> diff --git a/target/linux/ipq40xx/base-files/lib/upgrade/openmesh.sh 
> b/target/linux/ipq40xx/base-files/lib/upgrade/openmesh.sh
> index e313562017..8e02186eb8 100644
> --- a/target/linux/ipq40xx/base-files/lib/upgrade/openmesh.sh
> +++ b/target/linux/ipq40xx/base-files/lib/upgrade/openmesh.sh
> @@ -74,7 +74,7 @@ platform_do_upgrade_openmesh() {
>       #
> 
>       # take care of restoring a saved config
> -     [ "$UPGRADE_OPT_SAVE_CONFIG" -eq 1 ] && 
> restore_backup="${MTD_CONFIG_ARGS} -j ${UPGRADE_BACKUP}"
> +     [ -n "$UPGRADE_BACKUP" ] && restore_backup="${MTD_CONFIG_ARGS} -j 
> ${UPGRADE_BACKUP}"

"-f" here, too?

> 
>       mtd -q erase inactive
>       tar xf $tar_file ${board_dir}/root -O | mtd -n -p $kernel_length 
> $restore_backup write - $PART_NAME
> diff --git a/target/linux/ipq40xx/base-files/lib/upgrade/platform.sh 
> b/target/linux/ipq40xx/base-files/lib/upgrade/platform.sh
> index 6b9858beb0..c12508c437 100644
> --- a/target/linux/ipq40xx/base-files/lib/upgrade/platform.sh
> +++ b/target/linux/ipq40xx/base-files/lib/upgrade/platform.sh
> @@ -37,7 +37,7 @@ zyxel_do_upgrade() {
> 
>       tar Oxf $tar_file ${board_dir}/kernel | mtd write - kernel
> 
> -     if [ "$UPGRADE_OPT_SAVE_CONFIG" -eq 1 ]; then
> +     if [ -n "$UPGRADE_BACKUP" ]; then

"-f" here, too?

Sorry for being late with this.

Best

Adrian

>               tar Oxf $tar_file ${board_dir}/root | mtd -j "$UPGRADE_BACKUP" 
> write - rootfs
>       else
>               tar Oxf $tar_file ${board_dir}/root | mtd write - rootfs
> diff --git a/target/linux/ixp4xx/base-files/lib/upgrade/platform.sh 
> b/target/linux/ixp4xx/base-files/lib/upgrade/platform.sh
> index 557f43ce6f..f83aa430cf 100644
> --- a/target/linux/ixp4xx/base-files/lib/upgrade/platform.sh
> +++ b/target/linux/ixp4xx/base-files/lib/upgrade/platform.sh
> @@ -68,7 +68,7 @@ platform_do_upgrade_combined() {
>          [ ${erase_size:-0} -gt 0 ];
>       then
>               local append=""
> -             [ -f "$UPGRADE_BACKUP" -a "$UPGRADE_OPT_SAVE_CONFIG" -eq 1 ] && 
> append="-j $UPGRADE_BACKUP"
> +             [ -f "$UPGRADE_BACKUP" ] && append="-j $UPGRADE_BACKUP"
> 
>               # write the kernel
>               dd if="$1" bs=$CI_BLKSZ skip=1 count=$kern_blocks 2>/dev/null | 
> \
> --
> 2.21.0
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> [email protected]
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Attachment: openpgp-digital-signature.asc
Description: PGP signature

_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Reply via email to