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

[Simon Glass]

Hi Eugeniu,

On Wed, 22 May 2019 at 01:11, Eugeniu Rosca <erosca at de.adit-jv.com> wrote:
>
> 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/ ?

Yes

>
> 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?

I think code size in commands is not the major priority, although we
do tend to try to keep it moderate.

Here I wasn't suggesting removing code. I was just suggesting that the
error handling could be in each specific command's function. So take
the code out of each case statement and put into the function that it
relates to.

Or continue to use the generic error handler, but call it from the
generic function. It seems like we have:

do_bcb() {
   switch (cmd) {
   case CMD_FRED
      do_bcb_fred();
     ...
   }
  ...
}

do_bcb_fred() {
   check_args(CMD_FRED);
...
}

check_args(int cmd) {
   switch (cmd) {
      case CMD_FRED:
        print error
      }
   }


I mean, put 'print error' inside do_bcb_fred()

or, call check_args() from do_bcb()

>
> >
> > > +       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.

Yes this is good. See above where I tried to explain what I mean.
>
> > > +       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.

Yes, understood (I would calll this 'nul', BTW)

[..]

Regards,
Simon