On Fri, Jul 12, 2019 at 4:37 PM Sam Protsenko semen.protsenko@linaro.org wrote:

On Fri, Jul 12, 2019 at 3:50 PM Eugeniu Rosca erosca@de.adit-jv.com wrote:

...

Fix warning V1037 reported by PVS-Studio Static Analyzer: Two or more case-branches perform the same actions. Check lines: 49, 53

Fixes: db7b7a05b267 ("cmd: Add 'bcb' command to read/modify/write BCB fields") Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Reviewed-by: Igor Opaniuk igor.opaniuk@gmail.com

---

v2:

• Added 'Reviewed-by: Igor Opaniuk' from v1

v1:

• https://patchwork.ozlabs.org/patch/1131301/

---

Reviewed-by: Sam Protsenko semen.protsenko@linaro.org

...

cmd/bcb.c | 3 --- 1 file changed, 3 deletions(-)

diff --git a/cmd/bcb.c b/cmd/bcb.c index 2bd5a744deb5..3b1c7434e287 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -46,9 +46,6 @@ static int bcb_is_misused(int argc, char *const argv[])

    switch (cmd) {
    case BCB_CMD_LOAD:

•           if (argc != 3)
  

•                   goto err;
  

•           break;
  

Wait, one note here... Can you please add /* Fall through */ comment here? I remember that new GCC was complaining on cases without break that don't have such comment or corresponding fall-through attribute. I can see that U-Boot build doesn't emit any warnings for that case, but I think such comment would be useful anyway.

...

    case BCB_CMD_FIELD_SET:
            if (argc != 3)
                    goto err;

-- 2.22.0

On Fri, Jul 12, 2019 at 6:49 PM Eugeniu Rosca erosca@de.adit-jv.com wrote:

On Fri, Jul 12, 2019 at 05:01:09PM +0300, Sam Protsenko wrote:

...

...

...

diff --git a/cmd/bcb.c b/cmd/bcb.c index 2bd5a744deb5..3b1c7434e287 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -46,9 +46,6 @@ static int bcb_is_misused(int argc, char *const argv[])

    switch (cmd) {
    case BCB_CMD_LOAD:

•           if (argc != 3)
  

•                   goto err;
  

•           break;
  

Wait, one note here... Can you please add /* Fall through */ comment here? I remember that new GCC was complaining on cases without break that don't have such comment or corresponding fall-through attribute. I can see that U-Boot build doesn't emit any warnings for that case, but I think such comment would be useful anyway.

...

...

    case BCB_CMD_FIELD_SET:
            if (argc != 3)
                    goto err;

Both U-Boot and Linux enable [-Wimplicit-fallthrough=] on "make W=1". Regardless of U-Boot and Linux, gcc doesn't seem to report any warning when two case statements are stacked on top of each other without any logic in between. The warning is only triggered if there is minimal processing done in between (e.g. a dummy printf call).

If we still want the /* fallthrough */ comment, I will add it in the next patch revision.

Understood. Ok, let's not worry about this then. My R-b tag stands.

-- Best Regards, Eugeniu.

On Fri, Jul 12, 2019 at 3:50 PM Eugeniu Rosca erosca@de.adit-jv.com wrote:

Quote from https://patchwork.ozlabs.org/patch/1104244/#2210814:

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

Fixes: db7b7a05b267 ("cmd: Add 'bcb' command to read/modify/write BCB fields") Reported-by: Simon Glass sjg@chromium.org Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com

---

v2:

• Fixed accidental rename of field/size variables in bcb_field_get()

v1:

• https://patchwork.ozlabs.org/patch/1131306/

---

Reviewed-by: Sam Protsenko semen.protsenko@linaro.org

cmd/bcb.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/cmd/bcb.c b/cmd/bcb.c index 3b1c7434e287..fa9fdeeb0dfc 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -24,17 +24,17 @@ static struct bootloader_message bcb = { { 0 } };

static int bcb_cmd_get(char *cmd) {

•   if (!strncmp(cmd, "load", sizeof("load")))
  

•   if (!strcmp(cmd, "load"))
            return BCB_CMD_LOAD;
  

•   if (!strncmp(cmd, "set", sizeof("set")))
  

•   if (!strcmp(cmd, "set"))
            return BCB_CMD_FIELD_SET;
  

•   if (!strncmp(cmd, "clear", sizeof("clear")))
  

•   if (!strcmp(cmd, "clear"))
            return BCB_CMD_FIELD_CLEAR;
  

•   if (!strncmp(cmd, "test", sizeof("test")))
  

•   if (!strcmp(cmd, "test"))
            return BCB_CMD_FIELD_TEST;
  

•   if (!strncmp(cmd, "store", sizeof("store")))
  

•   if (!strcmp(cmd, "store"))
            return BCB_CMD_STORE;
  

•   if (!strncmp(cmd, "dump", sizeof("dump")))
  

•   if (!strcmp(cmd, "dump"))
            return BCB_CMD_FIELD_DUMP;
    else
            return -1;
  

@@ -85,19 +85,19 @@ err:

static int bcb_field_get(char *name, char **field, int *size) {

•   if (!strncmp(name, "command", sizeof("command"))) {
  

•   if (!strcmp(name, "command")) {
            *field = bcb.command;
            *size = sizeof(bcb.command);
  

•   } else if (!strncmp(name, "status", sizeof("status"))) {
  

•   } else if (!strcmp(name, "status")) {
            *field = bcb.status;
            *size = sizeof(bcb.status);
  

•   } else if (!strncmp(name, "recovery", sizeof("recovery"))) {
  

•   } else if (!strcmp(name, "recovery")) {
            *field = bcb.recovery;
            *size = sizeof(bcb.recovery);
  

•   } else if (!strncmp(name, "stage", sizeof("stage"))) {
  

•   } else if (!strcmp(name, "stage")) {
            *field = bcb.stage;
            *size = sizeof(bcb.stage);
  

•   } else if (!strncmp(name, "reserved", sizeof("reserved"))) {
  

•   } else if (!strcmp(name, "reserved")) {
            *field = bcb.reserved;
            *size = sizeof(bcb.reserved);
    } else {
  

-- 2.22.0

On Fri, Jul 12, 2019 at 3:52 PM Eugeniu Rosca erosca@de.adit-jv.com wrote:

These have been reported by Simon in [1] and fixed in [2]. However, since [1] has already been pushed to u-boot/master, the improvements incorporated in [2] are now extracted and resubmitted.

The changes are in the area of coding style and best practices:

• s/field/fieldp/, s/size/sizep/, to convey that the variables return an output to the caller
• s/err_1/err_read_fail/, s/err_2/err_too_small/, to be more descriptive
• Made sure 'static int do_bcb_load' appears on the same line
• Placed a `/*` on top of multi-line comment

[1] https://patchwork.ozlabs.org/patch/1104244/#2200259 [2] https://patchwork.ozlabs.org/cover/1128661/ ("[v4,0/4] Add 'bcb' command to read/modify/write Android BCB")

Fixes: db7b7a05b267 ("cmd: Add 'bcb' command to read/modify/write BCB fields") Reported-by: Simon Glass sjg@chromium.org Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com

---

v2:

• [Igor Opaniuk] Enriched the patch description
• Fixed inconsistent {field,size} variable rename

v1:

• https://patchwork.ozlabs.org/patch/1131321/

---

Reviewed-by: Sam Protsenko semen.protsenko@linaro.org

cmd/bcb.c | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/cmd/bcb.c b/cmd/bcb.c index fa9fdeeb0dfc..9626f2c69e34 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -83,23 +83,23 @@ err: return -1; }

-static int bcb_field_get(char *name, char **field, int *size) +static int bcb_field_get(char *name, char **fieldp, int *sizep) { if (!strcmp(name, "command")) {

•           *field = bcb.command;
  

•           *size = sizeof(bcb.command);
  

•           *fieldp = bcb.command;
  

•           *sizep = sizeof(bcb.command);
    } else if (!strcmp(name, "status")) {
  

•           *field = bcb.status;
  

•           *size = sizeof(bcb.status);
  

•           *fieldp = bcb.status;
  

•           *sizep = sizeof(bcb.status);
    } else if (!strcmp(name, "recovery")) {
  

•           *field = bcb.recovery;
  

•           *size = sizeof(bcb.recovery);
  

•           *fieldp = bcb.recovery;
  

•           *sizep = sizeof(bcb.recovery);
    } else if (!strcmp(name, "stage")) {
  

•           *field = bcb.stage;
  

•           *size = sizeof(bcb.stage);
  

•           *fieldp = bcb.stage;
  

•           *sizep = sizeof(bcb.stage);
    } else if (!strcmp(name, "reserved")) {
  

•           *field = bcb.reserved;
  

•           *size = sizeof(bcb.reserved);
  

•           *fieldp = bcb.reserved;
  

•           *sizep = sizeof(bcb.reserved);
    } else {
            printf("Error: Unknown bcb field '%s'\n", name);
            return -1;
  

@@ -108,8 +108,8 @@ static int bcb_field_get(char *name, char **field, int *size) return 0; }

-static int -do_bcb_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +static int do_bcb_load(cmd_tbl_t *cmdtp, int flag, int argc,

•                  char * const argv[])
  

{ struct blk_desc *desc; disk_partition_t info; @@ -119,28 +119,28 @@ do_bcb_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

    ret = blk_get_device_by_str("mmc", argv[1], &desc);
    if (ret < 0)

•           goto err_1;
  

•           goto err_read_fail;
  
    part = simple_strtoul(argv[2], &endp, 0);
    if (*endp == '\0') {
            ret = part_get_info(desc, part, &info);
            if (ret)
  

•                   goto err_1;
  

•                   goto err_read_fail;
    } else {
            part = part_get_info_by_name(desc, argv[2], &info);
            if (part < 0) {
                    ret = part;
  

•                   goto err_1;
  

•                   goto err_read_fail;
            }
    }
  
    cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz);
    if (cnt > info.size)
  

•           goto err_2;
  

•           goto err_too_small;
  
    if (blk_dread(desc, info.start, cnt, &bcb) != cnt) {
            ret = -EIO;
  

•           goto err_1;
  

•           goto err_read_fail;
    }
  
    bcb_dev = desc->devnum;
  

@@ -148,10 +148,10 @@ do_bcb_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) debug("%s: Loaded from mmc %d:%d\n", __func__, bcb_dev, bcb_part);

    return CMD_RET_SUCCESS;

-err_1: +err_read_fail: printf("Error: mmc %s:%s read failed (%d)\n", argv[1], argv[2], ret); goto err; -err_2: +err_too_small: printf("Error: mmc %s:%s too small!", argv[1], argv[2]); goto err; err: @@ -304,7 +304,8 @@ static int do_bcb(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_USAGE;

    if (bcb_is_misused(argc, argv)) {

•           /* We try to improve the user experience by reporting the
  

•           /*
  

•            * We try to improve the user experience by reporting the
             * root-cause of misusage, so don't return CMD_RET_USAGE,
             * since the latter prints out the full-blown help text
             */
  

-- 2.22.0