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@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;elsereturn -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 textRight, 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.