diff mbox series

[v2,4/6] base-files: fwtool: make compat_version backward compatible

Message ID 20200714142825.16889-5-freifunk@adrianschmutzler.de
State Accepted
Delegated to: Adrian Schmutzler
Headers show
Series sysupgrade: introduce compatibility version for devices | expand

Commit Message

Adrian Schmutzler July 14, 2020, 2:28 p.m. UTC 
So far, the compatibility mechanism only works if both device and
image are already updated to the new routines. This patch extends
the sysupgrade metadata and fwtool_check_image() to account for
"older" images as well:

The basic mechanism for older devices to check for image compatibility
is the supported_devices entry. This can be exploited by putting
a custom message into this variable of the metadata, so older FW
will produce a mismatch and print the message as it thinks it's the
list of supported devices. So, we have two cases:

device 1.0, image 1.0:
  The metadata will just contain supported_devices as before.

device 1.0, image 1.1:
  The metadata will contain:

  "new_supported_devices":["device_string1", "device_string2", ...],
  "supported_devices":["Upgrade incompatible. Please check Wiki ..."]

  If the device is "legacy", i.e. does not have the updated fwtool.sh,
  it will just fail with image check and print the content of
  supported_devices. Upgrade can still be performed with -F like when
  SUPPORTED_DEVICES has been removed to prevent bricking.

  If the device has updated fwtool.sh (but is 1.0), it will just use
  the new_supported_devices instead, and work as intended (flashing
  with -n will work, flashing without will print the appropriate
  warning).

This mechanism should provide a fair tradeoff between simplicity
and functionality.

Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>

---

Changes in v2:
- Add DEVICE_COMPAT_MESSAGE and reword comment for legacy warning.
---
 include/image-commands.mk                      | 5 ++++-
 package/base-files/files/lib/upgrade/fwtool.sh | 7 ++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

Comments

Paul Spooren July 16, 2020, 4:15 a.m. UTC | #1 
On 14.07.20 04:28, Adrian Schmutzler wrote:

> So far, the compatibility mechanism only works if both device and
> image are already updated to the new routines. This patch extends
> the sysupgrade metadata and fwtool_check_image() to account for
> "older" images as well:
>
> The basic mechanism for older devices to check for image compatibility
> is the supported_devices entry. This can be exploited by putting
> a custom message into this variable of the metadata, so older FW
> will produce a mismatch and print the message as it thinks it's the
> list of supported devices. So, we have two cases:
>
> device 1.0, image 1.0:
>    The metadata will just contain supported_devices as before.
>
> device 1.0, image 1.1:
>    The metadata will contain:
>
>    "new_supported_devices":["device_string1", "device_string2", ...],
>    "supported_devices":["Upgrade incompatible. Please check Wiki ..."]

Does that mean starting from 1.1 all sysupgrade images will carry a 
informative only "supported_device" array until the end of time?

This is maybe a bit of a big request but as there will be 19.07.4 
release without DSA, couldn't some firmware migration patches be added 
there and anyone trying to upgrade from 19.07.x will receive a message 
like "Please upgrade to 19.07.4 before installing 20.x"? Maybe it's not 
feasible, but as ideally all (correct?) devices move to DSA, we will 
have compat 1.1 everywhere and `supported_devices` is useless.

>    If the device is "legacy", i.e. does not have the updated fwtool.sh,
>    it will just fail with image check and print the content of
>    supported_devices. Upgrade can still be performed with -F like when
>    SUPPORTED_DEVICES has been removed to prevent bricking.
>
>    If the device has updated fwtool.sh (but is 1.0), it will just use
>    the new_supported_devices instead, and work as intended (flashing
>    with -n will work, flashing without will print the appropriate
>    warning).
>
> This mechanism should provide a fair tradeoff between simplicity
> and functionality.
>
> Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>
>
> ---
>
> Changes in v2:
> - Add DEVICE_COMPAT_MESSAGE and reword comment for legacy warning.
> ---
>   include/image-commands.mk                      | 5 ++++-
>   package/base-files/files/lib/upgrade/fwtool.sh | 7 ++++++-
>   2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/image-commands.mk b/include/image-commands.mk
> index 9da712e733..e21472a659 100644
> --- a/include/image-commands.mk
> +++ b/include/image-commands.mk
> @@ -393,7 +393,10 @@ metadata_json = \
>   		"metadata_version": "1.0", \
By replacing the meaning of `supported_devices` to 
`new_supported_devices` we should also modify `metadata_version`.
>   		"compat_version": "$(call json_quote,$(compat_version))", \
>   		$(if $(DEVICE_COMPAT_MESSAGE),"compat_message": "$(call json_quote,$(DEVICE_COMPAT_MESSAGE))"$(comma)) \
> -		"supported_devices":[$(call metadata_devices,$(SUPPORTED_DEVICES))], \
> +		$(if $(filter-out 1.0,$(compat_version)),"new_supported_devices":[$(call metadata_devices,$(SUPPORTED_DEVICES))]$(comma)) \
> +		$(if $(filter-out 1.0,$(compat_version)),"supported_devices": \
> +			["$(call json_quote,Image version $(compat_version) incompatible to device: $(if $(DEVICE_COMPAT_MESSAGE),$(DEVICE_COMPAT_MESSAGE),Please check documentation ...)"]$(comma)) \
> +		$(if $(filter 1.0,$(compat_version)),"supported_devices":[$(call metadata_devices,$(SUPPORTED_DEVICES))]$(comma)) \
>   		"version": { \
>   			"dist": "$(call json_quote,$(VERSION_DIST))", \
>   			"version": "$(call json_quote,$(VERSION_NUMBER))", \
> diff --git a/package/base-files/files/lib/upgrade/fwtool.sh b/package/base-files/files/lib/upgrade/fwtool.sh
> index 1807aecd6d..59f981f6c3 100644
> --- a/package/base-files/files/lib/upgrade/fwtool.sh
> +++ b/package/base-files/files/lib/upgrade/fwtool.sh
> @@ -51,7 +51,12 @@ fwtool_check_image() {
>   	json_get_var compatmessage compat_message
>   	[ -n "$imagecompat" ] || imagecompat="1.0"
>   
> -	json_select supported_devices || return 1
> +	# select correct supported list based on compat_version
> +	# (using this ensures that compatibility check works for devices
> +	#  not knowing about compat-version)
> +	local supported=supported_devices
> +	[ "$imagecompat" != "1.0" ] && supported=new_supported_devices
> +	json_select $supported || return 1
>   
>   	json_get_keys dev_keys
>   	for k in $dev_keys; do
Adrian Schmutzler July 16, 2020, 9:11 a.m. UTC | #2 
> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Paul Spooren
> Sent: Donnerstag, 16. Juli 2020 06:16
> To: Adrian Schmutzler <freifunk@adrianschmutzler.de>; openwrt-
> devel@lists.openwrt.org
> Subject: Re: [PATCH v2 4/6] base-files: fwtool: make compat_version
> backward compatible
> 
> On 14.07.20 04:28, Adrian Schmutzler wrote:
> 
> > So far, the compatibility mechanism only works if both device and
> > image are already updated to the new routines. This patch extends the
> > sysupgrade metadata and fwtool_check_image() to account for "older"
> > images as well:
> >
> > The basic mechanism for older devices to check for image compatibility
> > is the supported_devices entry. This can be exploited by putting a
> > custom message into this variable of the metadata, so older FW will
> > produce a mismatch and print the message as it thinks it's the list of
> > supported devices. So, we have two cases:
> >
> > device 1.0, image 1.0:
> >    The metadata will just contain supported_devices as before.
> >
> > device 1.0, image 1.1:
> >    The metadata will contain:
> >
> >    "new_supported_devices":["device_string1", "device_string2", ...],
> >    "supported_devices":["Upgrade incompatible. Please check Wiki ..."]
> 
> Does that mean starting from 1.1 all sysupgrade images will carry a
> informative only "supported_device" array until the end of time?

Yes. That's the only way I could think of to make this "backwards-compatible".
I just don't think it will hurt much, except for aesthetic reasons.

> This is maybe a bit of a big request but as there will be 19.07.4 release
> without DSA, couldn't some firmware migration patches be added there and
> anyone trying to upgrade from 19.07.x will receive a message like "Please
> upgrade to 19.07.4 before installing 20.x"? Maybe it's not feasible, but as

Despite, that I generally don't like the "update-to-X-first approach", I don't see how we will gain anything there for user before 19.07.4?
How would they receive the message then?

> ideally all (correct?) devices move to DSA, we will have compat 1.1
> everywhere and `supported_devices` is useless.

Generally yes, we have some devices that maybe won't be touched; but this change isn't really limited to DSA.
We just don't have any mechanism to prevent sysupgrade with kept config so far, and therefore I tried to implement this as general as possible.
Just think of the eth0/eth1 swap for ar71xx->ath79 without migration - that would be another example where we could have used such a mechanism if we already had it back then.
Maybe some devices will be at 1.3 in five years, as we introduce other incompatible changes.

It's just that so far we have nothing to treat incompatible changes, and I've been missing a comparable feature quite frequently already.

We just have to start at some point, and abusing supported_devices for backwards compatibility here seems to be the least painful way, as it won't hurt us practically, but just be a little bit ugly after all.

> 
> >    If the device is "legacy", i.e. does not have the updated fwtool.sh,
> >    it will just fail with image check and print the content of
> >    supported_devices. Upgrade can still be performed with -F like when
> >    SUPPORTED_DEVICES has been removed to prevent bricking.
> >
> >    If the device has updated fwtool.sh (but is 1.0), it will just use
> >    the new_supported_devices instead, and work as intended (flashing
> >    with -n will work, flashing without will print the appropriate
> >    warning).
> >
> > This mechanism should provide a fair tradeoff between simplicity and
> > functionality.
> >
> > Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>
> >
> > ---
> >
> > Changes in v2:
> > - Add DEVICE_COMPAT_MESSAGE and reword comment for legacy
> warning.
> > ---
> >   include/image-commands.mk                      | 5 ++++-
> >   package/base-files/files/lib/upgrade/fwtool.sh | 7 ++++++-
> >   2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/image-commands.mk b/include/image-commands.mk
> > index 9da712e733..e21472a659 100644
> > --- a/include/image-commands.mk
> > +++ b/include/image-commands.mk
> > @@ -393,7 +393,10 @@ metadata_json = \
> >   		"metadata_version": "1.0", \
> By replacing the meaning of `supported_devices` to
> `new_supported_devices` we should also modify `metadata_version`.

Fair point. I have to check where it's actually used.

Best

Adrian

> >   		"compat_version": "$(call json_quote,$(compat_version))", \
> >   		$(if $(DEVICE_COMPAT_MESSAGE),"compat_message":
> "$(call json_quote,$(DEVICE_COMPAT_MESSAGE))"$(comma)) \
> > -		"supported_devices":[$(call
> metadata_devices,$(SUPPORTED_DEVICES))], \
> > +		$(if $(filter-out
> 1.0,$(compat_version)),"new_supported_devices":[$(call
> metadata_devices,$(SUPPORTED_DEVICES))]$(comma)) \
> > +		$(if $(filter-out 1.0,$(compat_version)),"supported_devices":
> \
> > +			["$(call json_quote,Image version $(compat_version)
> incompatible to device: $(if
> $(DEVICE_COMPAT_MESSAGE),$(DEVICE_COMPAT_MESSAGE),Please check
> documentation ...)"]$(comma)) \
> > +		$(if $(filter
> 1.0,$(compat_version)),"supported_devices":[$(call
> > +metadata_devices,$(SUPPORTED_DEVICES))]$(comma)) \
> >   		"version": { \
> >   			"dist": "$(call json_quote,$(VERSION_DIST))", \
> >   			"version": "$(call
> json_quote,$(VERSION_NUMBER))", \ diff --git
> > a/package/base-files/files/lib/upgrade/fwtool.sh
> > b/package/base-files/files/lib/upgrade/fwtool.sh
> > index 1807aecd6d..59f981f6c3 100644
> > --- a/package/base-files/files/lib/upgrade/fwtool.sh
> > +++ b/package/base-files/files/lib/upgrade/fwtool.sh
> > @@ -51,7 +51,12 @@ fwtool_check_image() {
> >   	json_get_var compatmessage compat_message
> >   	[ -n "$imagecompat" ] || imagecompat="1.0"
> >
> > -	json_select supported_devices || return 1
> > +	# select correct supported list based on compat_version
> > +	# (using this ensures that compatibility check works for devices
> > +	#  not knowing about compat-version)
> > +	local supported=supported_devices
> > +	[ "$imagecompat" != "1.0" ] && supported=new_supported_devices
> > +	json_select $supported || return 1
> >
> >   	json_get_keys dev_keys
> >   	for k in $dev_keys; do
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff mbox series

Patch

diff --git a/include/image-commands.mk b/include/image-commands.mk
index 9da712e733..e21472a659 100644
--- a/include/image-commands.mk
+++ b/include/image-commands.mk
@@ -393,7 +393,10 @@  metadata_json = \
 		"metadata_version": "1.0", \
 		"compat_version": "$(call json_quote,$(compat_version))", \
 		$(if $(DEVICE_COMPAT_MESSAGE),"compat_message": "$(call json_quote,$(DEVICE_COMPAT_MESSAGE))"$(comma)) \
-		"supported_devices":[$(call metadata_devices,$(SUPPORTED_DEVICES))], \
+		$(if $(filter-out 1.0,$(compat_version)),"new_supported_devices":[$(call metadata_devices,$(SUPPORTED_DEVICES))]$(comma)) \
+		$(if $(filter-out 1.0,$(compat_version)),"supported_devices": \
+			["$(call json_quote,Image version $(compat_version) incompatible to device: $(if $(DEVICE_COMPAT_MESSAGE),$(DEVICE_COMPAT_MESSAGE),Please check documentation ...)"]$(comma)) \
+		$(if $(filter 1.0,$(compat_version)),"supported_devices":[$(call metadata_devices,$(SUPPORTED_DEVICES))]$(comma)) \
 		"version": { \
 			"dist": "$(call json_quote,$(VERSION_DIST))", \
 			"version": "$(call json_quote,$(VERSION_NUMBER))", \
diff --git a/package/base-files/files/lib/upgrade/fwtool.sh b/package/base-files/files/lib/upgrade/fwtool.sh
index 1807aecd6d..59f981f6c3 100644
--- a/package/base-files/files/lib/upgrade/fwtool.sh
+++ b/package/base-files/files/lib/upgrade/fwtool.sh
@@ -51,7 +51,12 @@  fwtool_check_image() {
 	json_get_var compatmessage compat_message
 	[ -n "$imagecompat" ] || imagecompat="1.0"
 
-	json_select supported_devices || return 1
+	# select correct supported list based on compat_version
+	# (using this ensures that compatibility check works for devices
+	#  not knowing about compat-version)
+	local supported=supported_devices
+	[ "$imagecompat" != "1.0" ] && supported=new_supported_devices
+	json_select $supported || return 1
 
 	json_get_keys dev_keys
 	for k in $dev_keys; do