[PATCH] cmd: part: number: remove inconsistent 0x from returned value

[Tom Rini]

On Fri, Mar 05, 2021 at 07:35:24AM -0700, Simon Glass wrote:
> Hi,
> 
> On Fri, 5 Mar 2021 at 07:33, Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
> >
> > Hi Eugeniu,
> >
> > Am 05.03.2021 um 12:52 schrieb Eugeniu Rosca:
> > > Hello Stefan,
> > >
> > > On Fri, Mar 05, 2021 at 07:39:04AM +0000, Stefan Herbrechtsmeier wrote:
> > >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
> > >>
> > >> The part number sub-command returns the hexadecimal value with a leading
> > >> 0x.
> > >
> > > That's to make sure that:
> > >     - users have clear and unequivocal feedback that '10'
> > >       returned by the command is really HEX 10, not DEC 10.
> > >     - other U-Boot commands which need to take '0x10' as input
> > >       will interpret it correctly, regardless of the way these
> > >       other commands implement ascii-to-integer conversion.
> >
> > 'Almost all U-Boot commands expect numbers to be entered in hexadecimal
> > input format.' [1]
> >
> > The filesystem commands use `simple_strtoul(.., 16)` and interpret the
> > value as hexadecimal value.
> >
> > The 0x suggests that a 10 will be interpreted as decimal value and this
> > isn't true.
> >
> > >> This is inconsistent with other values in the command
> > >
> > > It could be, but it is then better to fix the inconsistency in those
> > > commands/sub-commands which add the ambiguity.
> >
> > Normally you are right but U-Boot by design use hexadecimal values
> > without 0x. The env_set_hex functions doesn't use 0x.
> >
> > >> and U-Boot uses hexadecimal values generally.
> > >
> > > The key word is "generally", but not always. Some U-Boot commands will
> > > process '10' as HEX 10 and some will process 10 as DEC 10. So, in order
> > > to avoid these games, I vote for leaving the 0x in place.
> 
> I would be very surprised if 10 means 0d10 in a partition number. I
> agree that putting a 0x in these values is a dangerous precedent and
> will just cause confusion. U-Boot uses hex for addresses and most
> arguments
> 
> >
> > You can avoid it only if you could mark decimal numbers and that is
> > impossible.
> 
> 0d10 is available. People are not used to it though.
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> >
> > @Tom: Does U-Boot still expect numbers to be hexadecimal values?
> >
> > [1] https://www.denx.de/wiki/DULG/UBootCommandLineInterface
> >
> > >>
> > >> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
> > >>
> > >> ---
> > >>
> > >>   cmd/part.c | 2 +-
> > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/cmd/part.c b/cmd/part.c
> > >> index 3395c17b89..56e1852c66 100644
> > >> --- a/cmd/part.c
> > >> +++ b/cmd/part.c
> > >> @@ -152,7 +152,7 @@ static int do_part_info(int argc, char *const argv[], enum cmd_part_info param)
> > >>              snprintf(buf, sizeof(buf), LBAF, info.size);
> > >>              break;
> > >>      case CMD_PART_INFO_NUMBER:
> > >> -            snprintf(buf, sizeof(buf), "0x%x", part);
> > >> +            snprintf(buf, sizeof(buf), "%x", part);
> > >

I am not a fan of this change as well, especially having spent time on
some platforms that have literally 20+ partition entries.  Being clear
here that this is a hex value is important.


-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210306/a796e20d/attachment.sig>