On Wed, Mar 27, 2019 at 12:32 PM Eugeniu Rosca <ero...@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.kier...@gmail.com> wrote: > > > On Wed, Mar 27, 2019 at 1:04 AM Eugeniu Rosca <ero...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot