* [U-Boot] [PATCH] fastboot: getvar: fix broken 'fastboot flash' when partition has slots
@ 2019-03-27 1:02 Eugeniu Rosca
2019-03-27 5:44 ` Alex Kiernan
0 siblings, 1 reply; 6+ messages in thread
From: Eugeniu Rosca @ 2019-03-27 1:02 UTC (permalink / raw)
To: u-boot
With eMMC partitioning [1], 'fastboot getvar has-slot:<part-name>'
returns 'yes' only for 'system' and 'boot', while it is clear that [1]
has more partitions featuring slots (i.e. dtb, dtbo, vbmeta and vendor).
One not so obvious consequence is that
'fastboot flash {dtb,dtbo,vbmeta,vendor} img' will fail [2], while
'fastboot flash {boot,system} img' will succeed [3]. This is because
'fastboot flash' relies on the outcome of
'fastboot has-slot:<part-name>' behind the scene.
Assuming that the list of partitions featuring slots is vendor-
specific, U-Boot fastboot driver is not the best place for such list.
To avoid creating __weak functions overridden by board code, one simple
solution seems to be taking the user-provided partition as base, append
the "_a" suffix and checking if the result is a valid partition name.
This approach assumes that the 'has-slot:' API is specifically
designed to work with partition names chopped of their slot suffixes.
Reviewing the usage of 'has-slot' in upstream fastboot [4], this
assumption seems to be fortified, but to be 100% sure we need an Ack
from a fastboot expert.
[1] R-Car-H3 eMMC partitioning used for testing:
(bootloader) Created new GPT partition table:
(bootloader) /misc (512 KiB, raw)
(bootloader) /pst (512 KiB, raw)
(bootloader) /vbmeta_a (512 KiB, raw)
(bootloader) /vbmeta_b (512 KiB, raw)
(bootloader) /dtb_a (1024 KiB, raw)
(bootloader) /dtb_b (1024 KiB, raw)
(bootloader) /dtbo_a (512 KiB, raw)
(bootloader) /dtbo_b (512 KiB, raw)
(bootloader) /boot_a (16384 KiB, raw)
(bootloader) /boot_b (16384 KiB, raw)
(bootloader) /metadata (16384 KiB, raw)
(bootloader) /system_a (2301952 KiB, ext4)
(bootloader) /system_b (2301952 KiB, ext4)
(bootloader) /vendor_a (262144 KiB, ext4)
(bootloader) /vendor_b (262144 KiB, ext4)
(bootloader) /userdata (2451951 KiB, ext4)
[2] fastboot flash vbmeta vbmeta.img
target reported max download size of 16777216 bytes
Sending 'vbmeta' (4 KB)...
OKAY [ 0.025s]
Writing 'vbmeta'...
FAILED (remote: cannot find partition)
Finished. Total time: 0.332s
[3] fastboot flash boot boot.img
target reported max download size of 16777216 bytes
Sending 'boot_a' (16384 KB)...
OKAY [ 1.586s]
Writing 'boot_a'...
OKAY [ 0.379s]
Finished. Total time: 2.054s
[4] core (f959fffc1c8c) git grep -l has-slot
fastboot/constants.h
fastboot/fastboot.cpp
fastboot/fuzzy_fastboot/main.cpp
fs_mgr/tests/adb-remount-test.sh
Fixes: f73a7df984a9 ("net: fastboot: Merge AOSP UDP fastboot")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
drivers/fastboot/fb_getvar.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
index 4d264c985d7e..03bcd7162b37 100644
--- a/drivers/fastboot/fb_getvar.c
+++ b/drivers/fastboot/fb_getvar.c
@@ -130,8 +130,17 @@ static void getvar_slot_suffixes(char *var_parameter, char *response)
static void getvar_has_slot(char *part_name, char *response)
{
- if (part_name && (!strcmp(part_name, "boot") ||
- !strcmp(part_name, "system")))
+ struct blk_desc *dev_desc;
+ disk_partition_t info;
+ char name[32];
+
+ if (!part_name || !strcmp(part_name, ""))
+ return;
+
+ strlcpy(name, part_name, sizeof(name) - 2);
+ strcat(name, "_a");
+
+ if (fastboot_mmc_get_part_info(name, &dev_desc, &info, response) >= 0)
fastboot_okay("yes", response);
else
fastboot_okay("no", response);
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] fastboot: getvar: fix broken 'fastboot flash' when partition has slots
2019-03-27 1:02 [U-Boot] [PATCH] fastboot: getvar: fix broken 'fastboot flash' when partition has slots Eugeniu Rosca
@ 2019-03-27 5:44 ` Alex Kiernan
2019-03-27 5:55 ` Alex Kiernan
0 siblings, 1 reply; 6+ messages in thread
From: Alex Kiernan @ 2019-03-27 5:44 UTC (permalink / raw)
To: u-boot
On Wed, Mar 27, 2019 at 1:04 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> With eMMC partitioning [1], 'fastboot getvar has-slot:<part-name>'
> returns 'yes' only for 'system' and 'boot', while it is clear that [1]
> has more partitions featuring slots (i.e. dtb, dtbo, vbmeta and vendor).
>
> One not so obvious consequence is that
> 'fastboot flash {dtb,dtbo,vbmeta,vendor} img' will fail [2], while
> 'fastboot flash {boot,system} img' will succeed [3]. This is because
> 'fastboot flash' relies on the outcome of
> 'fastboot has-slot:<part-name>' behind the scene.
>
> Assuming that the list of partitions featuring slots is vendor-
> specific, U-Boot fastboot driver is not the best place for such list.
>
> To avoid creating __weak functions overridden by board code, one simple
> solution seems to be taking the user-provided partition as base, append
> the "_a" suffix and checking if the result is a valid partition name.
>
> This approach assumes that the 'has-slot:' API is specifically
> designed to work with partition names chopped of their slot suffixes.
> Reviewing the usage of 'has-slot' in upstream fastboot [4], this
> assumption seems to be fortified, but to be 100% sure we need an Ack
> from a fastboot expert.
>
> [1] R-Car-H3 eMMC partitioning used for testing:
> (bootloader) Created new GPT partition table:
> (bootloader) /misc (512 KiB, raw)
> (bootloader) /pst (512 KiB, raw)
> (bootloader) /vbmeta_a (512 KiB, raw)
> (bootloader) /vbmeta_b (512 KiB, raw)
> (bootloader) /dtb_a (1024 KiB, raw)
> (bootloader) /dtb_b (1024 KiB, raw)
> (bootloader) /dtbo_a (512 KiB, raw)
> (bootloader) /dtbo_b (512 KiB, raw)
> (bootloader) /boot_a (16384 KiB, raw)
> (bootloader) /boot_b (16384 KiB, raw)
> (bootloader) /metadata (16384 KiB, raw)
> (bootloader) /system_a (2301952 KiB, ext4)
> (bootloader) /system_b (2301952 KiB, ext4)
> (bootloader) /vendor_a (262144 KiB, ext4)
> (bootloader) /vendor_b (262144 KiB, ext4)
> (bootloader) /userdata (2451951 KiB, ext4)
>
> [2] fastboot flash vbmeta vbmeta.img
> target reported max download size of 16777216 bytes
> Sending 'vbmeta' (4 KB)...
> OKAY [ 0.025s]
> Writing 'vbmeta'...
> FAILED (remote: cannot find partition)
> Finished. Total time: 0.332s
>
> [3] fastboot flash boot boot.img
> target reported max download size of 16777216 bytes
> Sending 'boot_a' (16384 KB)...
> OKAY [ 1.586s]
> Writing 'boot_a'...
> OKAY [ 0.379s]
> Finished. Total time: 2.054s
>
> [4] core (f959fffc1c8c) git grep -l has-slot
> fastboot/constants.h
> fastboot/fastboot.cpp
> fastboot/fuzzy_fastboot/main.cpp
> fs_mgr/tests/adb-remount-test.sh
>
> Fixes: f73a7df984a9 ("net: fastboot: Merge AOSP UDP fastboot")
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> drivers/fastboot/fb_getvar.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> index 4d264c985d7e..03bcd7162b37 100644
> --- a/drivers/fastboot/fb_getvar.c
> +++ b/drivers/fastboot/fb_getvar.c
> @@ -130,8 +130,17 @@ static void getvar_slot_suffixes(char *var_parameter, char *response)
>
> static void getvar_has_slot(char *part_name, char *response)
> {
> - if (part_name && (!strcmp(part_name, "boot") ||
> - !strcmp(part_name, "system")))
> + struct blk_desc *dev_desc;
> + disk_partition_t info;
> + char name[32];
For the code as written this needs to be 33, or the strcat can
overflow by 1. Also we should use PART_NAME_LEN not 32 (fb_mmc.c needs
fixing in the same way).
> +
> + if (!part_name || !strcmp(part_name, ""))
> + return;
This needs to do fastboot_okay/fastboot_fail or you'll get the
protocol out of sync
> +
> + strlcpy(name, part_name, sizeof(name) - 2);
> + strcat(name, "_a");
> +
> + if (fastboot_mmc_get_part_info(name, &dev_desc, &info, response) >= 0)
> fastboot_okay("yes", response);
> else
> fastboot_okay("no", response);
This will fail if you're building with CONFIG_FASTBOOT_FLASH_NAND.
Also fastboot_mmc_get_part_info() sends its own fastboot_fail so this
just isn't going to work in a failure case.
> --
> 2.21.0
>
--
Alex Kiernan
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] fastboot: getvar: fix broken 'fastboot flash' when partition has slots
2019-03-27 5:44 ` Alex Kiernan
@ 2019-03-27 5:55 ` Alex Kiernan
2019-03-27 12:31 ` Eugeniu Rosca
0 siblings, 1 reply; 6+ messages in thread
From: Alex Kiernan @ 2019-03-27 5:55 UTC (permalink / raw)
To: u-boot
On Wed, Mar 27, 2019 at 5:44 AM Alex Kiernan <alex.kiernan@gmail.com> wrote:
>
> On Wed, Mar 27, 2019 at 1:04 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > With eMMC partitioning [1], 'fastboot getvar has-slot:<part-name>'
> > returns 'yes' only for 'system' and 'boot', while it is clear that [1]
> > has more partitions featuring slots (i.e. dtb, dtbo, vbmeta and vendor).
> >
> > One not so obvious consequence is that
> > 'fastboot flash {dtb,dtbo,vbmeta,vendor} img' will fail [2], while
> > 'fastboot flash {boot,system} img' will succeed [3]. This is because
> > 'fastboot flash' relies on the outcome of
> > 'fastboot has-slot:<part-name>' behind the scene.
> >
> > Assuming that the list of partitions featuring slots is vendor-
> > specific, U-Boot fastboot driver is not the best place for such list.
> >
> > To avoid creating __weak functions overridden by board code, one simple
> > solution seems to be taking the user-provided partition as base, append
> > the "_a" suffix and checking if the result is a valid partition name.
> >
> > This approach assumes that the 'has-slot:' API is specifically
> > designed to work with partition names chopped of their slot suffixes.
> > Reviewing the usage of 'has-slot' in upstream fastboot [4], this
> > assumption seems to be fortified, but to be 100% sure we need an Ack
> > from a fastboot expert.
> >
> > [1] R-Car-H3 eMMC partitioning used for testing:
> > (bootloader) Created new GPT partition table:
> > (bootloader) /misc (512 KiB, raw)
> > (bootloader) /pst (512 KiB, raw)
> > (bootloader) /vbmeta_a (512 KiB, raw)
> > (bootloader) /vbmeta_b (512 KiB, raw)
> > (bootloader) /dtb_a (1024 KiB, raw)
> > (bootloader) /dtb_b (1024 KiB, raw)
> > (bootloader) /dtbo_a (512 KiB, raw)
> > (bootloader) /dtbo_b (512 KiB, raw)
> > (bootloader) /boot_a (16384 KiB, raw)
> > (bootloader) /boot_b (16384 KiB, raw)
> > (bootloader) /metadata (16384 KiB, raw)
> > (bootloader) /system_a (2301952 KiB, ext4)
> > (bootloader) /system_b (2301952 KiB, ext4)
> > (bootloader) /vendor_a (262144 KiB, ext4)
> > (bootloader) /vendor_b (262144 KiB, ext4)
> > (bootloader) /userdata (2451951 KiB, ext4)
> >
> > [2] fastboot flash vbmeta vbmeta.img
> > target reported max download size of 16777216 bytes
> > Sending 'vbmeta' (4 KB)...
> > OKAY [ 0.025s]
> > Writing 'vbmeta'...
> > FAILED (remote: cannot find partition)
> > Finished. Total time: 0.332s
> >
> > [3] fastboot flash boot boot.img
> > target reported max download size of 16777216 bytes
> > Sending 'boot_a' (16384 KB)...
> > OKAY [ 1.586s]
> > Writing 'boot_a'...
> > OKAY [ 0.379s]
> > Finished. Total time: 2.054s
> >
> > [4] core (f959fffc1c8c) git grep -l has-slot
> > fastboot/constants.h
> > fastboot/fastboot.cpp
> > fastboot/fuzzy_fastboot/main.cpp
> > fs_mgr/tests/adb-remount-test.sh
> >
> > Fixes: f73a7df984a9 ("net: fastboot: Merge AOSP UDP fastboot")
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > ---
> > drivers/fastboot/fb_getvar.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> > index 4d264c985d7e..03bcd7162b37 100644
> > --- a/drivers/fastboot/fb_getvar.c
> > +++ b/drivers/fastboot/fb_getvar.c
> > @@ -130,8 +130,17 @@ static void getvar_slot_suffixes(char *var_parameter, char *response)
> >
> > static void getvar_has_slot(char *part_name, char *response)
> > {
> > - if (part_name && (!strcmp(part_name, "boot") ||
> > - !strcmp(part_name, "system")))
> > + struct blk_desc *dev_desc;
> > + disk_partition_t info;
> > + char name[32];
>
> For the code as written this needs to be 33, or the strcat can
> overflow by 1.
Sorry, wrong way around... can truncate the name by 1, not overflow.
> Also we should use PART_NAME_LEN not 32 (fb_mmc.c needs
> fixing in the same way).
>
> > +
> > + if (!part_name || !strcmp(part_name, ""))
> > + return;
>
> This needs to do fastboot_okay/fastboot_fail or you'll get the
> protocol out of sync
>
> > +
> > + strlcpy(name, part_name, sizeof(name) - 2);
> > + strcat(name, "_a");
> > +
> > + if (fastboot_mmc_get_part_info(name, &dev_desc, &info, response) >= 0)
> > fastboot_okay("yes", response);
> > else
> > fastboot_okay("no", response);
>
> This will fail if you're building with CONFIG_FASTBOOT_FLASH_NAND.
> Also fastboot_mmc_get_part_info() sends its own fastboot_fail so this
> just isn't going to work in a failure case.
>
> > --
> > 2.21.0
> >
>
>
> --
> Alex Kiernan
--
Alex Kiernan
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] fastboot: getvar: fix broken 'fastboot flash' when partition has slots
2019-03-27 5:55 ` Alex Kiernan
@ 2019-03-27 12:31 ` Eugeniu Rosca
2019-03-28 9:29 ` Alex Kiernan
0 siblings, 1 reply; 6+ messages in thread
From: Eugeniu Rosca @ 2019-03-27 12:31 UTC (permalink / raw)
To: u-boot
Hi Alex,
Thanks for the precious comments. Some remarks below.
On Wed, Mar 27, 2019 at 05:55:51AM +0000, Alex Kiernan wrote:
> On Wed, Mar 27, 2019 at 5:44 AM Alex Kiernan <alex.kiernan@gmail.com> wrote:
> > On Wed, Mar 27, 2019 at 1:04 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
[..]
> > > diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> > > index 4d264c985d7e..03bcd7162b37 100644
> > > --- a/drivers/fastboot/fb_getvar.c
> > > +++ b/drivers/fastboot/fb_getvar.c
> > > @@ -130,8 +130,17 @@ static void getvar_slot_suffixes(char *var_parameter, char *response)
> > >
> > > static void getvar_has_slot(char *part_name, char *response)
> > > {
> > > - if (part_name && (!strcmp(part_name, "boot") ||
> > > - !strcmp(part_name, "system")))
> > > + struct blk_desc *dev_desc;
> > > + disk_partition_t info;
> > > + char name[32];
> >
> > For the code as written this needs to be 33, or the strcat can
> > overflow by 1.
>
> Sorry, wrong way around... can truncate the name by 1, not overflow.
Applying below two-liner instrumentation on top of my patch:
diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
index c703be4cd61e..0149824c0d3d 100644
--- a/drivers/fastboot/fb_getvar.c
+++ b/drivers/fastboot/fb_getvar.c
@@ -138,7 +138,9 @@ static void getvar_has_slot(char *part_name, char *response)
return;
strlcpy(name, part_name, sizeof(name) - 2);
+ printf("%s: name: %s, strlen(name) %lu\n", __func__, name, strlen(name));
strcat(name, "_a");
+ printf("%s: name: %s, strlen(name) %lu\n", __func__, name, strlen(name));
if (fastboot_mmc_get_part_info(name, &dev_desc, &info, response) >= 0)
fastboot_okay("yes", response);
Passing a 32+ character part-name to 'getvar has-slot' results in:
$ host> fastboot getvar has-slot:0123456789abcdef0123456789abcdef0123456789abcdef
$ target>
getvar_has_slot: name: 0123456789abcdef0123456789abc, strlen(name) 29
getvar_has_slot: name: 0123456789abcdef0123456789abc_a, strlen(name) 31
From the above results, it looks to me that the partition name handling
(including deliberate string truncation done by strlcpy) works
correctly. I am still ready to rework/optimize the implementation if
you have any concerns/counter-proposals.
>
> > Also we should use PART_NAME_LEN not 32 (fb_mmc.c needs
> > fixing in the same way).
Agreed. Will be fixed in v2.
> > > +
> > > + if (!part_name || !strcmp(part_name, ""))
> > > + return;
> >
> > This needs to do fastboot_okay/fastboot_fail or you'll get the
> > protocol out of sync
The idea was to avoid bloating U-Boot with string literals, but I can
add a fastboot_fail() call if you wish so. IMO if the lack of
fastboot_okay/fastboot_fail at the end of dispatching a fastboot call
triggers undefined behavior on host side, then long-term this should
be fixed in the U-Boot fastboot dispatcher itself. FWIW, the current
behavior on my target is:
host $> fastboot getvar has-slot
getvar:has-slot FAILED (status malformed (0 bytes))
Finished. Total time: 0.039s
host $> fastboot getvar has-slot:
getvar:has-slot: FAILED (status malformed (0 bytes))
Finished. Total time: 0.039s
> > > +
> > > + strlcpy(name, part_name, sizeof(name) - 2);
> > > + strcat(name, "_a");
> > > +
> > > + if (fastboot_mmc_get_part_info(name, &dev_desc, &info, response) >= 0)
> > > fastboot_okay("yes", response);
> > > else
> > > fastboot_okay("no", response);
> >
> > This will fail if you're building with CONFIG_FASTBOOT_FLASH_NAND.
Agreed. Has to be fixed in v2. Do you have any preference between
the build-time "#if CONFIG_IS_ENABLED()" and the run-time "IS_ENABLED()"
which will be needed for NAND-specific handling? It is my feeling that
in Linux realm the run-time checks are greatly preferred. Here is some
feedback from Stephen Rothwell in https://lkml.org/lkml/2018/2/22/406:
-----8<-----
> I like IS_ENABLED() being used wherever
> possible because it allows us better compiler coverage (in the face
> of CONFIG options) even if the compiler then elides the actual code.
> It also breaks the code up less than #ifdef's.
-----8<-----
> > Also fastboot_mmc_get_part_info() sends its own fastboot_fail so this
> > just isn't going to work in a failure case.
I agree that, with the patch applied, getvar_has_slot() will potentially
override the response returned by fastboot_mmc_get_part_info(), but is
this really a problem? Technically, this doesn't look like a problem,
because we expect the 'strlcpy(response, tag, FASTBOOT_RESPONSE_LEN)'
call from fastboot_response() to successfully overwrite any
previously-stored response. The only problematic aspect is of somewhat
increased complexity, since we allow the higher-level fastboot layers
to redefine the status returned by the lower-level fastboot layers (hey,
this sounds quite naturally to me after all). This is something related
to internal implementation detail and I believe it is up to us to allow
or forbid/discourage this. Whichever is our preference, IMO this goes
beyond the scope of this patch. Calling fastboot_okay/fail currently
happens non-uniformly across several fastboot internal components/files,
so cleaning this up will require non-trivial amount of time.
Thanks again and looking forward to your feedback.
Best regards,
Eugeniu.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] fastboot: getvar: fix broken 'fastboot flash' when partition has slots
2019-03-27 12:31 ` Eugeniu Rosca
@ 2019-03-28 9:29 ` Alex Kiernan
2019-03-28 10:36 ` Eugeniu Rosca
0 siblings, 1 reply; 6+ messages in thread
From: Alex Kiernan @ 2019-03-28 9:29 UTC (permalink / raw)
To: u-boot
On Wed, Mar 27, 2019 at 12:32 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Alex,
>
> Thanks for the precious comments. Some remarks below.
>
> On Wed, Mar 27, 2019 at 05:55:51AM +0000, Alex Kiernan wrote:
> > On Wed, Mar 27, 2019 at 5:44 AM Alex Kiernan <alex.kiernan@gmail.com> wrote:
> > > On Wed, Mar 27, 2019 at 1:04 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> [..]
> > > > diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> > > > index 4d264c985d7e..03bcd7162b37 100644
> > > > --- a/drivers/fastboot/fb_getvar.c
> > > > +++ b/drivers/fastboot/fb_getvar.c
> > > > @@ -130,8 +130,17 @@ static void getvar_slot_suffixes(char *var_parameter, char *response)
> > > >
> > > > static void getvar_has_slot(char *part_name, char *response)
> > > > {
> > > > - if (part_name && (!strcmp(part_name, "boot") ||
> > > > - !strcmp(part_name, "system")))
> > > > + struct blk_desc *dev_desc;
> > > > + disk_partition_t info;
> > > > + char name[32];
> > >
> > > For the code as written this needs to be 33, or the strcat can
> > > overflow by 1.
> >
> > Sorry, wrong way around... can truncate the name by 1, not overflow.
>
> Applying below two-liner instrumentation on top of my patch:
>
> diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> index c703be4cd61e..0149824c0d3d 100644
> --- a/drivers/fastboot/fb_getvar.c
> +++ b/drivers/fastboot/fb_getvar.c
> @@ -138,7 +138,9 @@ static void getvar_has_slot(char *part_name, char *response)
> return;
>
> strlcpy(name, part_name, sizeof(name) - 2);
> + printf("%s: name: %s, strlen(name) %lu\n", __func__, name, strlen(name));
> strcat(name, "_a");
> + printf("%s: name: %s, strlen(name) %lu\n", __func__, name, strlen(name));
>
> if (fastboot_mmc_get_part_info(name, &dev_desc, &info, response) >= 0)
> fastboot_okay("yes", response);
>
> Passing a 32+ character part-name to 'getvar has-slot' results in:
> $ host> fastboot getvar has-slot:0123456789abcdef0123456789abcdef0123456789abcdef
> $ target>
> getvar_has_slot: name: 0123456789abcdef0123456789abc, strlen(name) 29
> getvar_has_slot: name: 0123456789abcdef0123456789abc_a, strlen(name) 31
>
> From the above results, it looks to me that the partition name handling
> (including deliberate string truncation done by strlcpy) works
> correctly. I am still ready to rework/optimize the implementation if
> you have any concerns/counter-proposals.
>
Looking at the rest of the code, there's confusion as to whether it's
expecting a +1 or not, given disk_partition.name[] is PART_NAME_LEN, I
think what you have is right.
> >
> > > Also we should use PART_NAME_LEN not 32 (fb_mmc.c needs
> > > fixing in the same way).
>
> Agreed. Will be fixed in v2.
>
> > > > +
> > > > + if (!part_name || !strcmp(part_name, ""))
> > > > + return;
> > >
> > > This needs to do fastboot_okay/fastboot_fail or you'll get the
> > > protocol out of sync
>
> The idea was to avoid bloating U-Boot with string literals, but I can
> add a fastboot_fail() call if you wish so. IMO if the lack of
> fastboot_okay/fastboot_fail at the end of dispatching a fastboot call
> triggers undefined behavior on host side, then long-term this should
> be fixed in the U-Boot fastboot dispatcher itself. FWIW, the current
> behavior on my target is:
>
> host $> fastboot getvar has-slot
> getvar:has-slot FAILED (status malformed (0 bytes))
> Finished. Total time: 0.039s
> host $> fastboot getvar has-slot:
> getvar:has-slot: FAILED (status malformed (0 bytes))
> Finished. Total time: 0.039s
>
"status malformed" is a pretty poor failure. The other thing is check
both the UDP and USB paths - the UDP path gets stuck in timeouts if
you don't send something which is within the protocol (though probably
not for this case).
I'd agree that the structure is awkward - if you're looking at
implementing `getvar all` then I expect it all have to be refactored
as my recollection is for multiple values you have to send them as
INFO frames.
> > > > +
> > > > + strlcpy(name, part_name, sizeof(name) - 2);
> > > > + strcat(name, "_a");
> > > > +
> > > > + if (fastboot_mmc_get_part_info(name, &dev_desc, &info, response) >= 0)
> > > > fastboot_okay("yes", response);
> > > > else
> > > > fastboot_okay("no", response);
> > >
> > > This will fail if you're building with CONFIG_FASTBOOT_FLASH_NAND.
>
> Agreed. Has to be fixed in v2. Do you have any preference between
> the build-time "#if CONFIG_IS_ENABLED()" and the run-time "IS_ENABLED()"
> which will be needed for NAND-specific handling? It is my feeling that
> in Linux realm the run-time checks are greatly preferred. Here is some
> feedback from Stephen Rothwell in https://lkml.org/lkml/2018/2/22/406:
>
I guess my preference is for consistency within a subsystem;
personally I prefer the IS_ENABLED() style, but I suspect there's some
refactoring needed to make that work.
> -----8<-----
> > I like IS_ENABLED() being used wherever
> > possible because it allows us better compiler coverage (in the face
> > of CONFIG options) even if the compiler then elides the actual code.
> > It also breaks the code up less than #ifdef's.
> -----8<-----
>
> > > Also fastboot_mmc_get_part_info() sends its own fastboot_fail so this
> > > just isn't going to work in a failure case.
>
> I agree that, with the patch applied, getvar_has_slot() will potentially
> override the response returned by fastboot_mmc_get_part_info(), but is
> this really a problem? Technically, this doesn't look like a problem,
> because we expect the 'strlcpy(response, tag, FASTBOOT_RESPONSE_LEN)'
> call from fastboot_response() to successfully overwrite any
> previously-stored response. The only problematic aspect is of somewhat
> increased complexity, since we allow the higher-level fastboot layers
> to redefine the status returned by the lower-level fastboot layers (hey,
> this sounds quite naturally to me after all). This is something related
> to internal implementation detail and I believe it is up to us to allow
> or forbid/discourage this. Whichever is our preference, IMO this goes
> beyond the scope of this patch. Calling fastboot_okay/fail currently
> happens non-uniformly across several fastboot internal components/files,
> so cleaning this up will require non-trivial amount of time.
>
I don't think we should be knowingly leaving traps for the unwary - if
the structure needs fixing, it needs fixing properly.
When I pulled in the UDP code, what started out as a trivial import
some external code turned into a wholesale refactor and reorganise of
the fastboot code so that there was just one application layer with
two underlying transports.
--
Alex Kiernan
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] fastboot: getvar: fix broken 'fastboot flash' when partition has slots
2019-03-28 9:29 ` Alex Kiernan
@ 2019-03-28 10:36 ` Eugeniu Rosca
0 siblings, 0 replies; 6+ messages in thread
From: Eugeniu Rosca @ 2019-03-28 10:36 UTC (permalink / raw)
To: u-boot
Hi Alex,
Thanks yet again for the timely and insightful replies.
I'll be sending out a v2 tackling your review findings as soon as I can.
Best regards,
Eugeniu.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-28 10:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-27 1:02 [U-Boot] [PATCH] fastboot: getvar: fix broken 'fastboot flash' when partition has slots Eugeniu Rosca
2019-03-27 5:44 ` Alex Kiernan
2019-03-27 5:55 ` Alex Kiernan
2019-03-27 12:31 ` Eugeniu Rosca
2019-03-28 9:29 ` Alex Kiernan
2019-03-28 10:36 ` Eugeniu Rosca
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.