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 <ero...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot