| Message ID | 20230102232534.592501-1-computersforpeace@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Petr Štetiar |
| Headers | show |
| Series | [1/8] base-files: Remove nand.sh dependency from emmc upgrade | expand |
On Mon, Jan 02, 2023 at 03:25:27PM -0800, Brian Norris wrote: > emmc_do_upgrade() relies on identify() from the nand.sh upgrade helper. > This only works because FEATURES=emmc targets also tend to include > FEATURES=nand. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > > .../base-files/files/lib/upgrade/common.sh | 27 ++++++++++++++++ > package/base-files/files/lib/upgrade/emmc.sh | 2 +- > package/base-files/files/lib/upgrade/nand.sh | 32 ++----------------- > 3 files changed, 30 insertions(+), 31 deletions(-) > > diff --git a/package/base-files/files/lib/upgrade/common.sh b/package/base-files/files/lib/upgrade/common.sh > index 5af061f6a439..53b8865a5788 100644 > --- a/package/base-files/files/lib/upgrade/common.sh > +++ b/package/base-files/files/lib/upgrade/common.sh > @@ -127,6 +127,33 @@ get_magic_fat32() { > (get_image "$@" | dd bs=1 count=5 skip=82) 2>/dev/null > } > > +identify_magic_long() { > + local magic=$1 > + case "$magic" in > + "55424923") > + echo "ubi" > + ;; > + "31181006") > + echo "ubifs" > + ;; > + "68737173") > + echo "squashfs" > + ;; > + "d00dfeed") > + echo "fit" > + ;; > + "4349"*) > + echo "combined" > + ;; > + "1f8b"*) > + echo "gzip" > + ;; > + *) > + echo "unknown $magic" > + ;; > + esac > +} > + > part_magic_efi() { > local magic=$(get_magic_gpt "$@") > [ "$magic" = "EFI PART" ] > diff --git a/package/base-files/files/lib/upgrade/emmc.sh b/package/base-files/files/lib/upgrade/emmc.sh > index c3b02864aa91..49cffe1c658b 100644 > --- a/package/base-files/files/lib/upgrade/emmc.sh > +++ b/package/base-files/files/lib/upgrade/emmc.sh > @@ -58,7 +58,7 @@ emmc_copy_config() { > } > > emmc_do_upgrade() { > - local file_type=$(identify $1) > + local file_type=$(identify_magic_long "$(get_magic_long "$1")") > > case "$file_type" in > "fit") emmc_upgrade_fit $1;; > diff --git a/package/base-files/files/lib/upgrade/nand.sh b/package/base-files/files/lib/upgrade/nand.sh > index a8e3cab0b8b1..a1dbd6e2663d 100644 > --- a/package/base-files/files/lib/upgrade/nand.sh > +++ b/package/base-files/files/lib/upgrade/nand.sh > @@ -65,40 +65,12 @@ get_magic_long_tar() { > (tar xO${3}f "$1" "$2" | dd bs=4 count=1 | hexdump -v -n 4 -e '1/1 "%02x"') 2> /dev/null > } > > -identify_magic() { > - local magic=$1 > - case "$magic" in > - "55424923") > - echo "ubi" > - ;; > - "31181006") > - echo "ubifs" > - ;; > - "68737173") > - echo "squashfs" > - ;; > - "d00dfeed") > - echo "fit" > - ;; > - "4349"*) > - echo "combined" > - ;; > - "1f8b"*) > - echo "gzip" > - ;; > - *) > - echo "unknown $magic" > - ;; > - esac > -} > - > - > identify() { > - identify_magic $(nand_get_magic_long "$@") > + identify_magic_long $(nand_get_magic_long "$@") Should we really change the name from identify_magic to identify_magic_long? > } > > identify_tar() { > - identify_magic $(get_magic_long_tar "$@") > + identify_magic_long $(get_magic_long_tar "$@") > } > > identify_if_gzip() { > -- > 2.39.0 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Hi Christian, On Wed, Jan 04, 2023 at 01:40:57PM +0100, Christian Marangi wrote: > On Mon, Jan 02, 2023 at 03:25:27PM -0800, Brian Norris wrote: > > --- a/package/base-files/files/lib/upgrade/common.sh > > +++ b/package/base-files/files/lib/upgrade/common.sh > > @@ -127,6 +127,33 @@ get_magic_fat32() { > > (get_image "$@" | dd bs=1 count=5 skip=82) 2>/dev/null > > } > > > > +identify_magic_long() { ... > > diff --git a/package/base-files/files/lib/upgrade/nand.sh b/package/base-files/files/lib/upgrade/nand.sh > > index a8e3cab0b8b1..a1dbd6e2663d 100644 > > --- a/package/base-files/files/lib/upgrade/nand.sh > > +++ b/package/base-files/files/lib/upgrade/nand.sh > > @@ -65,40 +65,12 @@ get_magic_long_tar() { > > identify() { > > - identify_magic $(nand_get_magic_long "$@") > > + identify_magic_long $(nand_get_magic_long "$@") > > Should we really change the name from identify_magic to > identify_magic_long? I was trying to match the conventions of common.sh, where I moved the helper to. common.sh has get_magic_word() and get_magic_long() (and other suffixed helpers for specific purposes), and it's important the caller uses the correct ones. While this identify function could partially work (it can identify "combined" and "gzip") with just a 'word' of data, it would be misleading. Is there a reason *not* to change the name? Thanks, Brian
On Wed, Jan 04, 2023 at 06:26:28PM -0800, Brian Norris wrote: > Hi Christian, > > On Wed, Jan 04, 2023 at 01:40:57PM +0100, Christian Marangi wrote: > > On Mon, Jan 02, 2023 at 03:25:27PM -0800, Brian Norris wrote: > > > --- a/package/base-files/files/lib/upgrade/common.sh > > > +++ b/package/base-files/files/lib/upgrade/common.sh > > > @@ -127,6 +127,33 @@ get_magic_fat32() { > > > (get_image "$@" | dd bs=1 count=5 skip=82) 2>/dev/null > > > } > > > > > > +identify_magic_long() { > ... > > > > diff --git a/package/base-files/files/lib/upgrade/nand.sh b/package/base-files/files/lib/upgrade/nand.sh > > > index a8e3cab0b8b1..a1dbd6e2663d 100644 > > > --- a/package/base-files/files/lib/upgrade/nand.sh > > > +++ b/package/base-files/files/lib/upgrade/nand.sh > > > @@ -65,40 +65,12 @@ get_magic_long_tar() { > > > > identify() { > > > - identify_magic $(nand_get_magic_long "$@") > > > + identify_magic_long $(nand_get_magic_long "$@") > > > > Should we really change the name from identify_magic to > > identify_magic_long? > > I was trying to match the conventions of common.sh, where I moved the > helper to. common.sh has get_magic_word() and get_magic_long() (and > other suffixed helpers for specific purposes), and it's important the > caller uses the correct ones. While this identify function could > partially work (it can identify "combined" and "gzip") with just a > 'word' of data, it would be misleading. > > Is there a reason *not* to change the name? > One of the reason was that change name of old function may cause some problem on downstream project... But looking at the script is full of inconsistency... Wonder If we should care to fix that. Anyway for me the name change it's OK. They are internal script so downstream project should update their script if they ever care to update to a new openwrt base. And no package should use the /lib/upgrade scripts to do stuff...
diff --git a/package/base-files/files/lib/upgrade/common.sh b/package/base-files/files/lib/upgrade/common.sh index 5af061f6a439..53b8865a5788 100644 --- a/package/base-files/files/lib/upgrade/common.sh +++ b/package/base-files/files/lib/upgrade/common.sh @@ -127,6 +127,33 @@ get_magic_fat32() { (get_image "$@" | dd bs=1 count=5 skip=82) 2>/dev/null } +identify_magic_long() { + local magic=$1 + case "$magic" in + "55424923") + echo "ubi" + ;; + "31181006") + echo "ubifs" + ;; + "68737173") + echo "squashfs" + ;; + "d00dfeed") + echo "fit" + ;; + "4349"*) + echo "combined" + ;; + "1f8b"*) + echo "gzip" + ;; + *) + echo "unknown $magic" + ;; + esac +} + part_magic_efi() { local magic=$(get_magic_gpt "$@") [ "$magic" = "EFI PART" ] diff --git a/package/base-files/files/lib/upgrade/emmc.sh b/package/base-files/files/lib/upgrade/emmc.sh index c3b02864aa91..49cffe1c658b 100644 --- a/package/base-files/files/lib/upgrade/emmc.sh +++ b/package/base-files/files/lib/upgrade/emmc.sh @@ -58,7 +58,7 @@ emmc_copy_config() { } emmc_do_upgrade() { - local file_type=$(identify $1) + local file_type=$(identify_magic_long "$(get_magic_long "$1")") case "$file_type" in "fit") emmc_upgrade_fit $1;; diff --git a/package/base-files/files/lib/upgrade/nand.sh b/package/base-files/files/lib/upgrade/nand.sh index a8e3cab0b8b1..a1dbd6e2663d 100644 --- a/package/base-files/files/lib/upgrade/nand.sh +++ b/package/base-files/files/lib/upgrade/nand.sh @@ -65,40 +65,12 @@ get_magic_long_tar() { (tar xO${3}f "$1" "$2" | dd bs=4 count=1 | hexdump -v -n 4 -e '1/1 "%02x"') 2> /dev/null } -identify_magic() { - local magic=$1 - case "$magic" in - "55424923") - echo "ubi" - ;; - "31181006") - echo "ubifs" - ;; - "68737173") - echo "squashfs" - ;; - "d00dfeed") - echo "fit" - ;; - "4349"*) - echo "combined" - ;; - "1f8b"*) - echo "gzip" - ;; - *) - echo "unknown $magic" - ;; - esac -} - - identify() { - identify_magic $(nand_get_magic_long "$@") + identify_magic_long $(nand_get_magic_long "$@") } identify_tar() { - identify_magic $(get_magic_long_tar "$@") + identify_magic_long $(get_magic_long_tar "$@") } identify_if_gzip() {
emmc_do_upgrade() relies on identify() from the nand.sh upgrade helper. This only works because FEATURES=emmc targets also tend to include FEATURES=nand. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- .../base-files/files/lib/upgrade/common.sh | 27 ++++++++++++++++ package/base-files/files/lib/upgrade/emmc.sh | 2 +- package/base-files/files/lib/upgrade/nand.sh | 32 ++----------------- 3 files changed, 30 insertions(+), 31 deletions(-)