mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH] fastboot: inform user if barebox update falls back to raw copy
@ 2022-05-31  9:35 Ahmad Fatoum
  2022-06-09  6:05 ` Ahmad Fatoum
  2022-06-09  7:19 ` Sascha Hauer
  0 siblings, 2 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2022-05-31  9:35 UTC (permalink / raw)
  To: barebox; +Cc: lgo, Ahmad Fatoum

Fastboot code determines whether to go into barebox update handler by
checking whether filetype_is_barebox_image() returns true for the image.
This is different from the barebox_update command which leaves it to the
individual handlers to decide whether an image is suitable.

Coupled with eMMC boot partitions, this could lead to
`barebox_update -t mmc` updating the boot partition, while a
`fastboot flash bbu-mmc` will write the image to the user area instead.

It may be worth breaking backwards compatibility here and refuse update
in that case, but for now at least alert the user that a raw copy is
being attempted.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/fastboot.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/common/fastboot.c b/common/fastboot.c
index f8ed40c86e00..50ea8421d92c 100644
--- a/common/fastboot.c
+++ b/common/fastboot.c
@@ -673,7 +673,7 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
 		goto out;
 	}
 
-	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
+	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE)) {
 		void *buf;
 		struct bbu_handler *handler;
 		struct bbu_data data = {
@@ -681,6 +681,17 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
 			.flags = BBU_FLAG_YES,
 		};
 
+		if (!filetype_is_barebox_image(filetype)) {
+			if (strstarts(fentry->name, "bbu-")) {
+				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
+						  "This is _not_ a barebox image...");
+				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
+						  "Falling back to raw copy to %s", filename);
+			}
+
+			goto copy;
+		}
+
 		handler = bbu_find_handler_by_device(data.devicefile);
 		if (!handler)
 			goto copy;
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fastboot: inform user if barebox update falls back to raw copy
  2022-05-31  9:35 [PATCH] fastboot: inform user if barebox update falls back to raw copy Ahmad Fatoum
@ 2022-06-09  6:05 ` Ahmad Fatoum
  2022-06-09  7:19 ` Sascha Hauer
  1 sibling, 0 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2022-06-09  6:05 UTC (permalink / raw)
  To: barebox; +Cc: lgo

Hello Sascha,

On 31.05.22 11:35, Ahmad Fatoum wrote:
> Fastboot code determines whether to go into barebox update handler by
> checking whether filetype_is_barebox_image() returns true for the image.
> This is different from the barebox_update command which leaves it to the
> individual handlers to decide whether an image is suitable.
> 
> Coupled with eMMC boot partitions, this could lead to
> `barebox_update -t mmc` updating the boot partition, while a
> `fastboot flash bbu-mmc` will write the image to the user area instead.
> 
> It may be worth breaking backwards compatibility here and refuse update
> in that case, but for now at least alert the user that a raw copy is
> being attempted.

Any comments?

> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/fastboot.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/common/fastboot.c b/common/fastboot.c
> index f8ed40c86e00..50ea8421d92c 100644
> --- a/common/fastboot.c
> +++ b/common/fastboot.c
> @@ -673,7 +673,7 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>  		goto out;
>  	}
>  
> -	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
> +	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE)) {
>  		void *buf;
>  		struct bbu_handler *handler;
>  		struct bbu_data data = {
> @@ -681,6 +681,17 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>  			.flags = BBU_FLAG_YES,
>  		};
>  
> +		if (!filetype_is_barebox_image(filetype)) {
> +			if (strstarts(fentry->name, "bbu-")) {
> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> +						  "This is _not_ a barebox image...");
> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> +						  "Falling back to raw copy to %s", filename);
> +			}
> +
> +			goto copy;
> +		}
> +
>  		handler = bbu_find_handler_by_device(data.devicefile);
>  		if (!handler)
>  			goto copy;


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fastboot: inform user if barebox update falls back to raw copy
  2022-05-31  9:35 [PATCH] fastboot: inform user if barebox update falls back to raw copy Ahmad Fatoum
  2022-06-09  6:05 ` Ahmad Fatoum
@ 2022-06-09  7:19 ` Sascha Hauer
  2022-06-09  8:21   ` Ahmad Fatoum
  1 sibling, 1 reply; 5+ messages in thread
From: Sascha Hauer @ 2022-06-09  7:19 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox, lgo

On Tue, May 31, 2022 at 11:35:37AM +0200, Ahmad Fatoum wrote:
> Fastboot code determines whether to go into barebox update handler by
> checking whether filetype_is_barebox_image() returns true for the image.
> This is different from the barebox_update command which leaves it to the
> individual handlers to decide whether an image is suitable.
> 
> Coupled with eMMC boot partitions, this could lead to
> `barebox_update -t mmc` updating the boot partition, while a
> `fastboot flash bbu-mmc` will write the image to the user area instead.
> 
> It may be worth breaking backwards compatibility here and refuse update
> in that case, but for now at least alert the user that a raw copy is
> being attempted.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/fastboot.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/common/fastboot.c b/common/fastboot.c
> index f8ed40c86e00..50ea8421d92c 100644
> --- a/common/fastboot.c
> +++ b/common/fastboot.c
> @@ -673,7 +673,7 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>  		goto out;
>  	}
>  
> -	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
> +	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE)) {


>  		void *buf;
>  		struct bbu_handler *handler;
>  		struct bbu_data data = {
> @@ -681,6 +681,17 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>  			.flags = BBU_FLAG_YES,
>  		};
>  
> +		if (!filetype_is_barebox_image(filetype)) {
> +			if (strstarts(fentry->name, "bbu-")) {
> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> +						  "This is _not_ a barebox image...");
> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> +						  "Falling back to raw copy to %s", filename);
> +			}
> +
> +			goto copy;
> +		}

The handlers starting with "bbu-" are registered from the barebox update
code, so falling back to copy is clearly wrong.

The logic to enter this barebox_update path should be:

	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) &&
	    (filetype_is_barebox_image(filetype) || strstarts(fentry->name, "bbu-")))

That way we are sure that barebox_update is called when we need it. Also
behaviour is consistent to what barebox_update does on the command line.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fastboot: inform user if barebox update falls back to raw copy
  2022-06-09  7:19 ` Sascha Hauer
@ 2022-06-09  8:21   ` Ahmad Fatoum
  2022-06-09  8:33     ` Sascha Hauer
  0 siblings, 1 reply; 5+ messages in thread
From: Ahmad Fatoum @ 2022-06-09  8:21 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox, lgo

Hello Sascha,

On 09.06.22 09:19, Sascha Hauer wrote:
> On Tue, May 31, 2022 at 11:35:37AM +0200, Ahmad Fatoum wrote:
>> Fastboot code determines whether to go into barebox update handler by
>> checking whether filetype_is_barebox_image() returns true for the image.
>> This is different from the barebox_update command which leaves it to the
>> individual handlers to decide whether an image is suitable.
>>
>> Coupled with eMMC boot partitions, this could lead to
>> `barebox_update -t mmc` updating the boot partition, while a
>> `fastboot flash bbu-mmc` will write the image to the user area instead.
>>
>> It may be worth breaking backwards compatibility here and refuse update
>> in that case, but for now at least alert the user that a raw copy is
>> being attempted.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  common/fastboot.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/fastboot.c b/common/fastboot.c
>> index f8ed40c86e00..50ea8421d92c 100644
>> --- a/common/fastboot.c
>> +++ b/common/fastboot.c
>> @@ -673,7 +673,7 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>>  		goto out;
>>  	}
>>  
>> -	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
>> +	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE)) {
> 
> 
>>  		void *buf;
>>  		struct bbu_handler *handler;
>>  		struct bbu_data data = {
>> @@ -681,6 +681,17 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>>  			.flags = BBU_FLAG_YES,
>>  		};
>>  
>> +		if (!filetype_is_barebox_image(filetype)) {
>> +			if (strstarts(fentry->name, "bbu-")) {
>> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
>> +						  "This is _not_ a barebox image...");
>> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
>> +						  "Falling back to raw copy to %s", filename);
>> +			}
>> +
>> +			goto copy;
>> +		}
> 
> The handlers starting with "bbu-" are registered from the barebox update
> code, so falling back to copy is clearly wrong.
> 
> The logic to enter this barebox_update path should be:
> 
> 	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) &&
> 	    (filetype_is_barebox_image(filetype) || strstarts(fentry->name, "bbu-")))
> 
> That way we are sure that barebox_update is called when we need it. Also
> behaviour is consistent to what barebox_update does on the command line.

This is not consistent with barebox_update behavior. Calling it from command line
will leave it to update handlers to determine whether an image is a barebox
image. filetype_is_barebox_image() is exclusively used for fastboot.

I don't like the current behavior either, but I am wary of breaking existing
users, so I figured an info message would be a stop-gap solution for now.

You can add a

  pr_warn("fastboot: deprecated raw copy fallback for barebox_update\n");

for now. What do you think?

Cheers,
Ahmad

> 
> Sascha
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fastboot: inform user if barebox update falls back to raw copy
  2022-06-09  8:21   ` Ahmad Fatoum
@ 2022-06-09  8:33     ` Sascha Hauer
  0 siblings, 0 replies; 5+ messages in thread
From: Sascha Hauer @ 2022-06-09  8:33 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox, lgo

On Thu, Jun 09, 2022 at 10:21:46AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 09.06.22 09:19, Sascha Hauer wrote:
> > On Tue, May 31, 2022 at 11:35:37AM +0200, Ahmad Fatoum wrote:
> >> Fastboot code determines whether to go into barebox update handler by
> >> checking whether filetype_is_barebox_image() returns true for the image.
> >> This is different from the barebox_update command which leaves it to the
> >> individual handlers to decide whether an image is suitable.
> >>
> >> Coupled with eMMC boot partitions, this could lead to
> >> `barebox_update -t mmc` updating the boot partition, while a
> >> `fastboot flash bbu-mmc` will write the image to the user area instead.
> >>
> >> It may be worth breaking backwards compatibility here and refuse update
> >> in that case, but for now at least alert the user that a raw copy is
> >> being attempted.
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >>  common/fastboot.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/common/fastboot.c b/common/fastboot.c
> >> index f8ed40c86e00..50ea8421d92c 100644
> >> --- a/common/fastboot.c
> >> +++ b/common/fastboot.c
> >> @@ -673,7 +673,7 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
> >>  		goto out;
> >>  	}
> >>  
> >> -	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
> >> +	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE)) {
> > 
> > 
> >>  		void *buf;
> >>  		struct bbu_handler *handler;
> >>  		struct bbu_data data = {
> >> @@ -681,6 +681,17 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
> >>  			.flags = BBU_FLAG_YES,
> >>  		};
> >>  
> >> +		if (!filetype_is_barebox_image(filetype)) {
> >> +			if (strstarts(fentry->name, "bbu-")) {
> >> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> >> +						  "This is _not_ a barebox image...");
> >> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> >> +						  "Falling back to raw copy to %s", filename);
> >> +			}
> >> +
> >> +			goto copy;
> >> +		}
> > 
> > The handlers starting with "bbu-" are registered from the barebox update
> > code, so falling back to copy is clearly wrong.
> > 
> > The logic to enter this barebox_update path should be:
> > 
> > 	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) &&
> > 	    (filetype_is_barebox_image(filetype) || strstarts(fentry->name, "bbu-")))
> > 
> > That way we are sure that barebox_update is called when we need it. Also
> > behaviour is consistent to what barebox_update does on the command line.
> 
> This is not consistent with barebox_update behavior. Calling it from command line
> will leave it to update handlers to determine whether an image is a barebox
> image. filetype_is_barebox_image() is exclusively used for fastboot.

With my suggestion we also leave it to the update handlers to decide
whether an image is suitable or not.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-06-09  8:49 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-31  9:35 [PATCH] fastboot: inform user if barebox update falls back to raw copy Ahmad Fatoum
2022-06-09  6:05 ` Ahmad Fatoum
2022-06-09  7:19 ` Sascha Hauer
2022-06-09  8:21   ` Ahmad Fatoum
2022-06-09  8:33     ` Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox