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

[Eugeniu Rosca]

Hi Simon,

On Sat, Jun 22, 2019 at 08:09:48PM +0100, Simon Glass wrote:
> On Thu, 23 May 2019 at 16:33, Eugeniu Rosca <erosca at de.adit-jv.com> wrote:

[..]

> I'm going to make some general comments as I still feel that this code
> is really odd.

Thanks. My replies below.

> > +enum bcb_cmd {
> 
> > +       BCB_CMD_LOAD,
> > +       BCB_CMD_FIELD_SET,
> > +       BCB_CMD_FIELD_CLEAR,
> > +       BCB_CMD_FIELD_TEST,
> > +       BCB_CMD_FIELD_DUMP,
> > +       BCB_CMD_STORE,
> > +};
> 
> It looks like this ^^ can be removed, see below.

My reply below.

> > +static int bcb_cmd_get(char *cmd)
> > +{
> > +       if (!strncmp(cmd, "load", sizeof("load")))
> 
> Is this strncmp() for a security reason? I don't think that is
> necessary. We typically would avoid using the command line in secure
> situations.

strncmp() is chosen for the sake of paranoid/defensive programming.
Indeed, strncmp() is not really needed when comparing a variable
with a string literal. We expect strcmp() to behave safely even if the
string variable is not NUL-terminated.

In the same scenario, Linux v5.2-rc7 uses both strcmp() and strncmp(),
but the frequency of strcmp() is higher:

$ git --version
git version 2.22.0
$ (Linux 5.2-rc7) git grep -En 'strncmp\([^"]*"[[:alnum:]]+"' | wc -l
1066
$ (Linux 5.2-rc7) git grep -En 'strcmp\([^"]*"[[:alnum:]]+"' | wc -l
1968

A quick "strcmp vs strncmp" object size test shows that strcmp()
generates smaller memory footprint (gcc-8, x86_64):

$ (U-Boot) size cmd/bcb-strncmp.o cmd/bcb-strcmp.o
   text	   data	    bss	    dec	    hex	filename
   3373	    400	   2048	   5821	   16bd	cmd/bcb-strncmp.o
   3314	    400	   2048	   5762	   1682	cmd/bcb-strcmp.o

So, overall, I agree to use strcmp() whenever variables are compared
with string literals.

> 
> Better I think to check just the first 1-2 chars which is enough to
> distinguish the command.

I personally think this will make the code less readable and will
increase maintenance effort. This will especially be annoying when
differentiating sub-commands like set/see, start/status, etc, which
have a long common prefix.

> 
> > +               return BCB_CMD_LOAD;
> > +       if (!strncmp(cmd, "set", sizeof("set")))
> > +               return BCB_CMD_FIELD_SET;
> > +       if (!strncmp(cmd, "clear", sizeof("clear")))
> > +               return BCB_CMD_FIELD_CLEAR;
> > +       if (!strncmp(cmd, "test", sizeof("test")))
> > +               return BCB_CMD_FIELD_TEST;
> > +       if (!strncmp(cmd, "store", sizeof("store")))
> > +               return BCB_CMD_STORE;
> > +       if (!strncmp(cmd, "dump", sizeof("dump")))
> > +               return BCB_CMD_FIELD_DUMP;
> > +       else
> > +               return -1;
> > +}
> 
> and this function ^^

Do you propose zapping both bcb_cmd_get() and bcb_is_misused()?

I stress again that bcb_is_misused() aims to centralize error reporting.
Without bcb_is_misused(), I would need to create multiple instances of
below error messages (since they would need to be reported by several
sub-commands), increasing the code size:

 - printf("Error: Please, load BCB first!\n");
 - printf("Error: Bad usage of 'bcb %s'\n", subcommand);

> 
> > +
> > +static int bcb_is_misused(int argc, char *const argv[])
> > +{
> > +       int cmd = bcb_cmd_get(argv[0]);
> > +
> > +       switch (cmd) {
> > +       case BCB_CMD_LOAD:
> > +               if (argc != 3)
> > +                       goto err;
> > +               break;
> 
> To me it does not make sense to hold the validation code for 'load'
> here. It just makes it harder to maintain if we update it, since we
> need to change the code and and below.

Please, make me understand. Are you unhappy about 'bcb load' only or
are you unhappy about the bcb_is_misused()? I provided some arguments
above to still keep this function in place.

If this function is preserved, I see no reason to treat 'bcb load'
differently from other sub-commands.

> > +static int bcb_field_get(char *name, char **field, int *size)
> 
> How about fieldp and sizep to indicate that these values are returned?

Makes sense. Will be updated.

> > +
> > +static int
> 
> Would prefer not to split the line here so we can search for 'int
> do_', for example.

Will update that, although I see 355 occurrences of this pattern in
U-Boot and 23980 occurrences in Linux v5.2-rc7:

$ (U-Boot/master) git grep -E "^static\s+[[:alnum:]]+$" -- "*.c"  "*.h"  | wc -l
355

$ (Linux v5.2-rc7) git grep -E "^static\s+[[:alnum:]]+$" -- "*.c"  "*.h"  | wc -l
23980

This makes me wonder where the border between subjective preferences
of the maintainer and objective coding style requirements really is?

> > +       if (blk_dread(desc, info.start, cnt, &bcb) != cnt) {
> > +               ret = -EIO;
> 
> Actually the error code is the return value of blk_dread(). Although
> perhaps it is badly documented.

Based on my reading, blk_dread() returns ulong, not a negative error
code. I could find no blk_dread() caller which reuses the return value
as error code. I will make no change here.

> > +err_1:
> 
> How about err_read_fail
> 
> > +err_2:
> 
> err_too_small

Agreed. Will be updated.

> 
> > +       printf("Error: mmc %s:%s too small!", argv[1], argv[2]);
> > +       goto err;
> > +err:
> > +       bcb_dev = -1;
> > +       bcb_part = -1;
> 
> Why these two lines?

They act as an indicator for wrongly loaded or unloaded BCB.

> > +       if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt) {
> 
> as above

My feedback on blk_dread() applies here too.

> > +       if (bcb_is_misused(argc, argv)) {
> > +               /* We try to improve the user experience by reporting the
> 
> /*
>  * We try ...
>  * ...
>  */

Will be updated.

> 
> > +                * root-cause of misusage, so don't return CMD_RET_USAGE,
> > +                * since the latter prints out the full-blown help text
> 
> Right, but that's the idea...this is how people get the syntax.

I prefer to tell the users what's the reason of their command being
rejected rather than throwing a full-blown help message which they
didn't ask for.

-- 
Best Regards,
Eugeniu.