| Message ID | 20211216102638.111391-1-matthias.schiffer@ew.tq-group.com |
|---|---|
| State | Accepted |
| Commit | 7e90f771730001f9ba749985f81103930e892eaf |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fastboot: only look up real partition names when no alias exists | expand |
Hi Matthias, On 12/16/21 5:26 AM, Matthias Schiffer wrote: > Having U-Boot look up the passed partition name even though an alias > exists is unexpected, leading to warning messages (when the alias name > doesn't exist as a real partition name) or the use of the wrong > partition. > > Change part_get_info_by_name_or_alias() to consider real partitions > names only if no alias of the same name exists, allowing to use aliases > to override the configuration for existing partition names. Much saner IMO. I think the correct move in the long term is to add a "quiet" parameter to do_get_part_info (and all its helpers). This is OK as an incremental improvement. > Also change one use of strcpy() to strlcpy(). > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > --- > drivers/fastboot/fb_mmc.c | 29 ++++++++++++----------------- > 1 file changed, 12 insertions(+), 17 deletions(-) > > diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c > index 2738dc836e..fb7791d9da 100644 > --- a/drivers/fastboot/fb_mmc.c > +++ b/drivers/fastboot/fb_mmc.c > @@ -104,23 +104,18 @@ static int part_get_info_by_name_or_alias(struct blk_desc **dev_desc, > const char *name, > struct disk_partition *info) > { > - int ret; > - > - ret = do_get_part_info(dev_desc, name, info); > - if (ret < 0) { > - /* strlen("fastboot_partition_alias_") + PART_NAME_LEN + 1 */ > - char env_alias_name[25 + PART_NAME_LEN + 1]; > - char *aliased_part_name; > - > - /* check for alias */ > - strcpy(env_alias_name, "fastboot_partition_alias_"); > - strlcat(env_alias_name, name, sizeof(env_alias_name)); > - aliased_part_name = env_get(env_alias_name); > - if (aliased_part_name != NULL) > - ret = do_get_part_info(dev_desc, aliased_part_name, > - info); > - } > - return ret; > + /* strlen("fastboot_partition_alias_") + PART_NAME_LEN + 1 */ > + char env_alias_name[25 + PART_NAME_LEN + 1]; > + char *aliased_part_name; > + > + /* check for alias */ > + strlcpy(env_alias_name, "fastboot_partition_alias_", sizeof(env_alias_name)); > + strlcat(env_alias_name, name, sizeof(env_alias_name)); > + aliased_part_name = env_get(env_alias_name); > + if (aliased_part_name) > + name = aliased_part_name; > + > + return do_get_part_info(dev_desc, name, info); > } > > /** > Reviewed-by: Sean Anderson <sean.anderson@seco.com> --Sean
On Fri, 2021-12-17 at 18:20 -0500, Sean Anderson wrote: > Hi Matthias, > > On 12/16/21 5:26 AM, Matthias Schiffer wrote: > > Having U-Boot look up the passed partition name even though an > > alias > > exists is unexpected, leading to warning messages (when the alias > > name > > doesn't exist as a real partition name) or the use of the wrong > > partition. > > > > Change part_get_info_by_name_or_alias() to consider real partitions > > names only if no alias of the same name exists, allowing to use > > aliases > > to override the configuration for existing partition names. > > Much saner IMO. > > I think the correct move in the long term is to add a "quiet" > parameter to do_get_part_info (and all its helpers). This is OK as an > incremental improvement. > > > Also change one use of strcpy() to strlcpy(). > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com > > > > > --- > > drivers/fastboot/fb_mmc.c | 29 ++++++++++++----------------- > > 1 file changed, 12 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c > > index 2738dc836e..fb7791d9da 100644 > > --- a/drivers/fastboot/fb_mmc.c > > +++ b/drivers/fastboot/fb_mmc.c > > @@ -104,23 +104,18 @@ static int > > part_get_info_by_name_or_alias(struct blk_desc **dev_desc, > > const char *name, > > struct disk_partition *info) > > { > > - int ret; > > - > > - ret = do_get_part_info(dev_desc, name, info); > > - if (ret < 0) { > > - /* strlen("fastboot_partition_alias_") + PART_NAME_LEN > > + 1 */ > > - char env_alias_name[25 + PART_NAME_LEN + 1]; > > - char *aliased_part_name; > > - > > - /* check for alias */ > > - strcpy(env_alias_name, "fastboot_partition_alias_"); > > - strlcat(env_alias_name, name, sizeof(env_alias_name)); > > - aliased_part_name = env_get(env_alias_name); > > - if (aliased_part_name != NULL) > > - ret = do_get_part_info(dev_desc, > > aliased_part_name, > > - info); > > - } > > - return ret; > > + /* strlen("fastboot_partition_alias_") + PART_NAME_LEN + 1 */ > > + char env_alias_name[25 + PART_NAME_LEN + 1]; > > + char *aliased_part_name; > > + > > + /* check for alias */ > > + strlcpy(env_alias_name, "fastboot_partition_alias_", > > sizeof(env_alias_name)); > > + strlcat(env_alias_name, name, sizeof(env_alias_name)); > > + aliased_part_name = env_get(env_alias_name); > > + if (aliased_part_name) > > + name = aliased_part_name; > > + > > + return do_get_part_info(dev_desc, name, info); > > } > > > > /** > > > > Reviewed-by: Sean Anderson <sean.anderson@seco.com> > > --Sean Can we get this committed? Kind regards, Matthias
On 1/26/22 4:54 AM, Matthias Schiffer wrote: > On Fri, 2021-12-17 at 18:20 -0500, Sean Anderson wrote: >> Hi Matthias, >> >> On 12/16/21 5:26 AM, Matthias Schiffer wrote: >> > Having U-Boot look up the passed partition name even though an >> > alias >> > exists is unexpected, leading to warning messages (when the alias >> > name >> > doesn't exist as a real partition name) or the use of the wrong >> > partition. >> > >> > Change part_get_info_by_name_or_alias() to consider real partitions >> > names only if no alias of the same name exists, allowing to use >> > aliases >> > to override the configuration for existing partition names. >> >> Much saner IMO. >> >> I think the correct move in the long term is to add a "quiet" >> parameter to do_get_part_info (and all its helpers). This is OK as an >> incremental improvement. >> >> > Also change one use of strcpy() to strlcpy(). >> > >> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com >> > > >> > --- >> > drivers/fastboot/fb_mmc.c | 29 ++++++++++++----------------- >> > 1 file changed, 12 insertions(+), 17 deletions(-) >> > >> > diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c >> > index 2738dc836e..fb7791d9da 100644 >> > --- a/drivers/fastboot/fb_mmc.c >> > +++ b/drivers/fastboot/fb_mmc.c >> > @@ -104,23 +104,18 @@ static int >> > part_get_info_by_name_or_alias(struct blk_desc **dev_desc, >> > const char *name, >> > struct disk_partition *info) >> > { >> > - int ret; >> > - >> > - ret = do_get_part_info(dev_desc, name, info); >> > - if (ret < 0) { >> > - /* strlen("fastboot_partition_alias_") + PART_NAME_LEN >> > + 1 */ >> > - char env_alias_name[25 + PART_NAME_LEN + 1]; >> > - char *aliased_part_name; >> > - >> > - /* check for alias */ >> > - strcpy(env_alias_name, "fastboot_partition_alias_"); >> > - strlcat(env_alias_name, name, sizeof(env_alias_name)); >> > - aliased_part_name = env_get(env_alias_name); >> > - if (aliased_part_name != NULL) >> > - ret = do_get_part_info(dev_desc, >> > aliased_part_name, >> > - info); >> > - } >> > - return ret; >> > + /* strlen("fastboot_partition_alias_") + PART_NAME_LEN + 1 */ >> > + char env_alias_name[25 + PART_NAME_LEN + 1]; >> > + char *aliased_part_name; >> > + >> > + /* check for alias */ >> > + strlcpy(env_alias_name, "fastboot_partition_alias_", >> > sizeof(env_alias_name)); >> > + strlcat(env_alias_name, name, sizeof(env_alias_name)); >> > + aliased_part_name = env_get(env_alias_name); >> > + if (aliased_part_name) >> > + name = aliased_part_name; >> > + >> > + return do_get_part_info(dev_desc, name, info); >> > } >> > >> > /** >> > >> >> Reviewed-by: Sean Anderson <sean.anderson@seco.com> >> >> --Sean > > > Can we get this committed? +CC Tom
On Thu, Jan 27, 2022 at 11:22:12AM -0500, Sean Anderson wrote: > > > On 1/26/22 4:54 AM, Matthias Schiffer wrote: > > On Fri, 2021-12-17 at 18:20 -0500, Sean Anderson wrote: > >> Hi Matthias, > >> > >> On 12/16/21 5:26 AM, Matthias Schiffer wrote: > >> > Having U-Boot look up the passed partition name even though an > >> > alias > >> > exists is unexpected, leading to warning messages (when the alias > >> > name > >> > doesn't exist as a real partition name) or the use of the wrong > >> > partition. > >> > > >> > Change part_get_info_by_name_or_alias() to consider real partitions > >> > names only if no alias of the same name exists, allowing to use > >> > aliases > >> > to override the configuration for existing partition names. > >> > >> Much saner IMO. > >> > >> I think the correct move in the long term is to add a "quiet" > >> parameter to do_get_part_info (and all its helpers). This is OK as an > >> incremental improvement. > >> > >> > Also change one use of strcpy() to strlcpy(). > >> > > >> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com > >> > > > >> > --- > >> > drivers/fastboot/fb_mmc.c | 29 ++++++++++++----------------- > >> > 1 file changed, 12 insertions(+), 17 deletions(-) > >> > > >> > diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c > >> > index 2738dc836e..fb7791d9da 100644 > >> > --- a/drivers/fastboot/fb_mmc.c > >> > +++ b/drivers/fastboot/fb_mmc.c > >> > @@ -104,23 +104,18 @@ static int > >> > part_get_info_by_name_or_alias(struct blk_desc **dev_desc, > >> > const char *name, > >> > struct disk_partition *info) > >> > { > >> > - int ret; > >> > - > >> > - ret = do_get_part_info(dev_desc, name, info); > >> > - if (ret < 0) { > >> > - /* strlen("fastboot_partition_alias_") + PART_NAME_LEN > >> > + 1 */ > >> > - char env_alias_name[25 + PART_NAME_LEN + 1]; > >> > - char *aliased_part_name; > >> > - > >> > - /* check for alias */ > >> > - strcpy(env_alias_name, "fastboot_partition_alias_"); > >> > - strlcat(env_alias_name, name, sizeof(env_alias_name)); > >> > - aliased_part_name = env_get(env_alias_name); > >> > - if (aliased_part_name != NULL) > >> > - ret = do_get_part_info(dev_desc, > >> > aliased_part_name, > >> > - info); > >> > - } > >> > - return ret; > >> > + /* strlen("fastboot_partition_alias_") + PART_NAME_LEN + 1 */ > >> > + char env_alias_name[25 + PART_NAME_LEN + 1]; > >> > + char *aliased_part_name; > >> > + > >> > + /* check for alias */ > >> > + strlcpy(env_alias_name, "fastboot_partition_alias_", > >> > sizeof(env_alias_name)); > >> > + strlcat(env_alias_name, name, sizeof(env_alias_name)); > >> > + aliased_part_name = env_get(env_alias_name); > >> > + if (aliased_part_name) > >> > + name = aliased_part_name; > >> > + > >> > + return do_get_part_info(dev_desc, name, info); > >> > } > >> > > >> > /** > >> > > >> > >> Reviewed-by: Sean Anderson <sean.anderson@seco.com> > >> > >> --Sean > > > > > > Can we get this committed? > > +CC Tom There's a number of fastboot things and I guess I need to see what I can try and review and grab or reassign.
On Thu, Dec 16, 2021 at 11:26:38AM +0100, Matthias Schiffer wrote: > Having U-Boot look up the passed partition name even though an alias > exists is unexpected, leading to warning messages (when the alias name > doesn't exist as a real partition name) or the use of the wrong > partition. > > Change part_get_info_by_name_or_alias() to consider real partitions > names only if no alias of the same name exists, allowing to use aliases > to override the configuration for existing partition names. > > Also change one use of strcpy() to strlcpy(). > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > Reviewed-by: Sean Anderson <sean.anderson@seco.com> Applied to u-boot/master, thanks!
diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c index 2738dc836e..fb7791d9da 100644 --- a/drivers/fastboot/fb_mmc.c +++ b/drivers/fastboot/fb_mmc.c @@ -104,23 +104,18 @@ static int part_get_info_by_name_or_alias(struct blk_desc **dev_desc, const char *name, struct disk_partition *info) { - int ret; - - ret = do_get_part_info(dev_desc, name, info); - if (ret < 0) { - /* strlen("fastboot_partition_alias_") + PART_NAME_LEN + 1 */ - char env_alias_name[25 + PART_NAME_LEN + 1]; - char *aliased_part_name; - - /* check for alias */ - strcpy(env_alias_name, "fastboot_partition_alias_"); - strlcat(env_alias_name, name, sizeof(env_alias_name)); - aliased_part_name = env_get(env_alias_name); - if (aliased_part_name != NULL) - ret = do_get_part_info(dev_desc, aliased_part_name, - info); - } - return ret; + /* strlen("fastboot_partition_alias_") + PART_NAME_LEN + 1 */ + char env_alias_name[25 + PART_NAME_LEN + 1]; + char *aliased_part_name; + + /* check for alias */ + strlcpy(env_alias_name, "fastboot_partition_alias_", sizeof(env_alias_name)); + strlcat(env_alias_name, name, sizeof(env_alias_name)); + aliased_part_name = env_get(env_alias_name); + if (aliased_part_name) + name = aliased_part_name; + + return do_get_part_info(dev_desc, name, info); } /**
Having U-Boot look up the passed partition name even though an alias exists is unexpected, leading to warning messages (when the alias name doesn't exist as a real partition name) or the use of the wrong partition. Change part_get_info_by_name_or_alias() to consider real partitions names only if no alias of the same name exists, allowing to use aliases to override the configuration for existing partition names. Also change one use of strcpy() to strlcpy(). Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- drivers/fastboot/fb_mmc.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-)