[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

Hi Simon,

Thanks for the review. Would you please reply to my questions below?

On Tue, May 21, 2019 at 06:53:29PM -0600, Simon Glass wrote:
> Hi Eugeniu,
> 
> On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca at de.adit-jv.com> wrote:
> >
> >
> > [1] https://android.googlesource.com/platform/bootable/recovery
> > [2] https://source.android.com/devices/bootloader
> > [3] https://patchwork.ozlabs.org/patch/746835/
> >     ("[U-Boot,5/6] Initial support for the Android Bootloader flow")
> 
> As discussed, please add these docs somewhere in the U-Boot tree (can
> be in a separate patch)

Sure.

> > +       if (blk_get_device_by_str("mmc", argv[1], &desc) < 0)
> 
> Error codes should be reported.

> > +       printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]);
> 
> Add error code here.

Will address.

> > +       switch (bcb_cmd_get(argv[0])) {
> 
> Why have a switch() here, when you could just put the below code in
> each function? Or put the call to this function from the main
> do_bcb_set() function.

s/do_bcb_set/do_bcb/ ?

The switch() is there to tell the user that he misused specifically
{sub}command 'bcb X'. If we just throw CMD_RET_USAGE (which means
printing full-blown help text) on _any_ kind of command misuse , we
don't help the user _at all_, IMHO. I would consider being user-friendly
as the higher and more important policy. However, if you prioritize the
code size over user experience, then I am open to rewrite the function.
Would you please clarify which policy takes precedence between the two?

> 
> > +       str = strdup(argv[2]);
> 
> It is OK to change the args if you like.

I will try getting rid of strdup.

> > +       if (bcb_is_misused(argc, argv))
> > +               return CMD_RET_FAILURE;
> 
> You should return CMD_RET_USAGE if there is a usage problem.

This again connects with my previous statement on the user-experience.
I would like to tell the user where exactly he did the mistake in using
the 'bcb' rather than throwing a full-blown help message.

> > +       if (bcb_field_get(argv[1], &field, &size))
> > +               return CMD_RET_FAILURE;
> > +
> > +       print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16);
> 
> Please add newline before return

Fine.

> > +       if (!strncmp(argv[2], "=", sizeof("="))) {
> 
> Think about code size:
> 
> if (*argv[2] == '=')

Checking a single character will not detect the difference between
'=' and '=anything' So, I have to validate, in addition, that the
NULL terminator is there. But I agree with the comment in general.

> > +       if (part_get_info(desc, bcb_part, &info))
> > +               goto err;
> 
> Again, error numbers need to be printed.

Will address.

> Regards,
> Simon

Thanks!

-- 
Best Regards,
Eugeniu.