[U-Boot] [PATCH 0/5] Fixes and improvements in BCB and Android docs

[U-Boot] [PATCH 0/5] Fixes and improvements in BCB and Android docs

[Eugeniu Rosca]

Resubmit the diff between [1] and [2] of the BCB/Android series, since
the former has been pushed to master instead of the latter.

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

Eugeniu Rosca (5):
  doc: Move README.android-fastboot-protocol to doc/android/
  treewide: Fix stale references of Android docs
  cmd: bcb: Fix duplicated handling in two case-branches
  cmd: bcb: Use strcmp() instead of strncmp() for string literals
  cmd: bcb: Apply non-functional refinements

 cmd/Kconfig                                   |  2 +-
 cmd/bcb.c                                     | 68 +++++++++----------
 .../fastboot-protocol.txt}                    |  0
 doc/android/fastboot.txt                      |  2 +-
 test/py/tests/test_avb.py                     |  2 +-
 5 files changed, 36 insertions(+), 38 deletions(-)
 rename doc/{README.android-fastboot-protocol => android/fastboot-protocol.txt} (100%)

-- 
2.22.0

[U-Boot] [PATCH 1/5] doc: Move README.android-fastboot-protocol to doc/android/

[Eugeniu Rosca]

Commit 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB
overview") missed relocating README.android-fastboot-protocol. Fix it.

Fixes: 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB overview")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 .../fastboot-protocol.txt}                                        | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename doc/{README.android-fastboot-protocol => android/fastboot-protocol.txt} (100%)

diff --git a/doc/README.android-fastboot-protocol b/doc/android/fastboot-protocol.txt
similarity index 100%
rename from doc/README.android-fastboot-protocol
rename to doc/android/fastboot-protocol.txt
-- 
2.22.0

[U-Boot] [PATCH 1/5] doc: Move README.android-fastboot-protocol to doc/android/

[Igor Opaniuk]

Hi Eugeniu,

On Fri, Jul 12, 2019 at 2:21 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Commit 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB
> overview") missed relocating README.android-fastboot-protocol. Fix it.
>
> Fixes: 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB overview")
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  .../fastboot-protocol.txt}                                        | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  rename doc/{README.android-fastboot-protocol => android/fastboot-protocol.txt} (100%)
>
> diff --git a/doc/README.android-fastboot-protocol b/doc/android/fastboot-protocol.txt
> similarity index 100%
> rename from doc/README.android-fastboot-protocol
> rename to doc/android/fastboot-protocol.txt
> --
> 2.22.0
>

Reviewed-by: Igor Opaniuk <igor.opaniuk@gmail.com>

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk

[U-Boot] [PATCH 1/5] doc: Move README.android-fastboot-protocol to doc/android/

[Sam Protsenko]

On Fri, Jul 12, 2019 at 2:23 PM Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
>
> Hi Eugeniu,
>
> On Fri, Jul 12, 2019 at 2:21 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > Commit 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB
> > overview") missed relocating README.android-fastboot-protocol. Fix it.
> >
> > Fixes: 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB overview")
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > ---
> >  .../fastboot-protocol.txt}                                        | 0
> >  1 file changed, 0 insertions(+), 0 deletions(-)
> >  rename doc/{README.android-fastboot-protocol => android/fastboot-protocol.txt} (100%)
> >
> > diff --git a/doc/README.android-fastboot-protocol b/doc/android/fastboot-protocol.txt
> > similarity index 100%
> > rename from doc/README.android-fastboot-protocol
> > rename to doc/android/fastboot-protocol.txt
> > --
> > 2.22.0
> >
>
> Reviewed-by: Igor Opaniuk <igor.opaniuk@gmail.com>
>

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

> --
> Best regards - Freundliche Grüsse - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opaniuk at gmail.com
> skype: igor.opanyuk
> +380 (93) 836 40 67
> http://ua.linkedin.com/in/iopaniuk

[U-Boot] [PATCH 1/5] doc: Move README.android-fastboot-protocol to doc/android/

[Eugeniu Rosca]

Superseded by https://patchwork.ozlabs.org/patch/1131355/
("[v2,1/5] doc: Move README.android-fastboot-protocol to doc/android/")

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 1/5] doc: Move README.android-fastboot-protocol to doc/android/

[Tom Rini]

On Sat, Jul 13, 2019 at 10:39:19AM +0200, Eugeniu Rosca wrote:

> Superseded by https://patchwork.ozlabs.org/patch/1131355/
> ("[v2,1/5] doc: Move README.android-fastboot-protocol to doc/android/")

Thanks!  This does help me avoid cases where I put up the old versions
of series (and thanks also for doing the v3->v4 fixup stuff for me).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190714/2540a313/attachment.sig>

[U-Boot] [PATCH 1/5] doc: Move README.android-fastboot-protocol to doc/android/

[Eugeniu Rosca]

Hi Tom,
cc: Jeremy, Stephen,

On Sun, Jul 14, 2019 at 01:14:32PM -0400, Tom Rini wrote:
> On Sat, Jul 13, 2019 at 10:39:19AM +0200, Eugeniu Rosca wrote:
> 
> > Superseded by https://patchwork.ozlabs.org/patch/1131355/
> > ("[v2,1/5] doc: Move README.android-fastboot-protocol to doc/android/")
> 
> Thanks!  This does help me avoid cases where I put up the old versions
> of series

I will then do it on regular basis and will encourage everybody else to
do the same. I think a simple bot (or patchwork itself) could implement
some heuristics to detect newer patch revisions and perform this linkage
(at some point in future).

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 2/5] treewide: Fix stale references of Android docs

[Eugeniu Rosca]

Commit 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB
overview") left some obsolete references of Android documents/paths.

This has been pointed out by Sam (thanks!) in:
https://patchwork.ozlabs.org/patch/1104245/#2208134

Fixes: 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB overview")
Reported-by: Sam Protsenko <semen.protsenko@linaro.org>
Suggested-by: Sam Protsenko <semen.protsenko@linaro.org>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 cmd/Kconfig               | 2 +-
 doc/android/fastboot.txt  | 2 +-
 test/py/tests/test_avb.py | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 3afb760a816f..3cf8233df62e 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -707,7 +707,7 @@ config CMD_FASTBOOT
 	  Android devices. Fastboot requires either the network stack
 	  enabled or support for acting as a USB device.
 
-	  See doc/README.android-fastboot for more information.
+	  See doc/android/fastboot.txt for more information.
 
 config CMD_FDC
 	bool "fdcboot - Boot from floppy device"
diff --git a/doc/android/fastboot.txt b/doc/android/fastboot.txt
index 431191c473f2..dcf824713f94 100644
--- a/doc/android/fastboot.txt
+++ b/doc/android/fastboot.txt
@@ -6,7 +6,7 @@ Overview
 ========
 
 The protocol that is used over USB and UDP is described in the
-``README.android-fastboot-protocol`` file in the same directory.
+``fastboot-protocol.txt`` file in the same directory.
 
 The current implementation supports the following standard commands:
 
diff --git a/test/py/tests/test_avb.py b/test/py/tests/test_avb.py
index 2bb75ed6e2a2..813242343555 100644
--- a/test/py/tests/test_avb.py
+++ b/test/py/tests/test_avb.py
@@ -8,7 +8,7 @@
 This tests Android Verified Boot 2.0 support in U-boot:
 
 For additional details about how to build proper vbmeta partition
-check doc/README.avb2
+check doc/android/avb2.txt
 
 For configuration verification:
 - Corrupt boot partition and check for failure
-- 
2.22.0

[U-Boot] [PATCH 2/5] treewide: Fix stale references of Android docs

[Igor Opaniuk]

On Fri, Jul 12, 2019 at 2:21 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Commit 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB
> overview") left some obsolete references of Android documents/paths.
>
> This has been pointed out by Sam (thanks!) in:
> https://patchwork.ozlabs.org/patch/1104245/#2208134
>
> Fixes: 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB overview")
> Reported-by: Sam Protsenko <semen.protsenko@linaro.org>
> Suggested-by: Sam Protsenko <semen.protsenko@linaro.org>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  cmd/Kconfig               | 2 +-
>  doc/android/fastboot.txt  | 2 +-
>  test/py/tests/test_avb.py | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 3afb760a816f..3cf8233df62e 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -707,7 +707,7 @@ config CMD_FASTBOOT
>           Android devices. Fastboot requires either the network stack
>           enabled or support for acting as a USB device.
>
> -         See doc/README.android-fastboot for more information.
> +         See doc/android/fastboot.txt for more information.
>
>  config CMD_FDC
>         bool "fdcboot - Boot from floppy device"
> diff --git a/doc/android/fastboot.txt b/doc/android/fastboot.txt
> index 431191c473f2..dcf824713f94 100644
> --- a/doc/android/fastboot.txt
> +++ b/doc/android/fastboot.txt
> @@ -6,7 +6,7 @@ Overview
>  ========
>
>  The protocol that is used over USB and UDP is described in the
> -``README.android-fastboot-protocol`` file in the same directory.
> +``fastboot-protocol.txt`` file in the same directory.
>
>  The current implementation supports the following standard commands:
>
> diff --git a/test/py/tests/test_avb.py b/test/py/tests/test_avb.py
> index 2bb75ed6e2a2..813242343555 100644
> --- a/test/py/tests/test_avb.py
> +++ b/test/py/tests/test_avb.py
> @@ -8,7 +8,7 @@
>  This tests Android Verified Boot 2.0 support in U-boot:
>
>  For additional details about how to build proper vbmeta partition
> -check doc/README.avb2
> +check doc/android/avb2.txt
>
>  For configuration verification:
>  - Corrupt boot partition and check for failure
> --
> 2.22.0
>

Reviewed-by: Igor Opaniuk <igor.opaniuk@gmail.com>

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk

[U-Boot] [PATCH 2/5] treewide: Fix stale references of Android docs

[Sam Protsenko]

On Fri, Jul 12, 2019 at 2:25 PM Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
>
> On Fri, Jul 12, 2019 at 2:21 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > Commit 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB
> > overview") left some obsolete references of Android documents/paths.
> >
> > This has been pointed out by Sam (thanks!) in:
> > https://patchwork.ozlabs.org/patch/1104245/#2208134
> >
> > Fixes: 9bdf0e8fef86 ("doc: relocate/rename Android README and add BCB overview")
> > Reported-by: Sam Protsenko <semen.protsenko@linaro.org>
> > Suggested-by: Sam Protsenko <semen.protsenko@linaro.org>
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > ---
> >  cmd/Kconfig               | 2 +-
> >  doc/android/fastboot.txt  | 2 +-
> >  test/py/tests/test_avb.py | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 3afb760a816f..3cf8233df62e 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -707,7 +707,7 @@ config CMD_FASTBOOT
> >           Android devices. Fastboot requires either the network stack
> >           enabled or support for acting as a USB device.
> >
> > -         See doc/README.android-fastboot for more information.
> > +         See doc/android/fastboot.txt for more information.
> >
> >  config CMD_FDC
> >         bool "fdcboot - Boot from floppy device"
> > diff --git a/doc/android/fastboot.txt b/doc/android/fastboot.txt
> > index 431191c473f2..dcf824713f94 100644
> > --- a/doc/android/fastboot.txt
> > +++ b/doc/android/fastboot.txt
> > @@ -6,7 +6,7 @@ Overview
> >  ========
> >
> >  The protocol that is used over USB and UDP is described in the
> > -``README.android-fastboot-protocol`` file in the same directory.
> > +``fastboot-protocol.txt`` file in the same directory.
> >
> >  The current implementation supports the following standard commands:
> >
> > diff --git a/test/py/tests/test_avb.py b/test/py/tests/test_avb.py
> > index 2bb75ed6e2a2..813242343555 100644
> > --- a/test/py/tests/test_avb.py
> > +++ b/test/py/tests/test_avb.py
> > @@ -8,7 +8,7 @@
> >  This tests Android Verified Boot 2.0 support in U-boot:
> >
> >  For additional details about how to build proper vbmeta partition
> > -check doc/README.avb2
> > +check doc/android/avb2.txt
> >
> >  For configuration verification:
> >  - Corrupt boot partition and check for failure
> > --
> > 2.22.0
> >
>
> Reviewed-by: Igor Opaniuk <igor.opaniuk@gmail.com>
>

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

> --
> Best regards - Freundliche Grüsse - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opaniuk at gmail.com
> skype: igor.opanyuk
> +380 (93) 836 40 67
> http://ua.linkedin.com/in/iopaniuk

[U-Boot] [PATCH 2/5] treewide: Fix stale references of Android docs

[Eugeniu Rosca]

Hi Sam,

On Fri, Jul 12, 2019 at 04:25:11PM +0300, Sam Protsenko wrote:
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

Please, discard the v1 [*] and consider v2 [**] instead.

There should be a clear protocol to communicate that to the maintainers,
such that we avoid seeing the v1 applied to master. I placed a comment
on the v1 [*] patchwork front-page. I hope that's enough.

[*] https://patchwork.ozlabs.org/cover/1131295/
[**] https://patchwork.ozlabs.org/cover/1131356/

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 2/5] treewide: Fix stale references of Android docs

[Eugeniu Rosca]

Superseded by https://patchwork.ozlabs.org/patch/1131354/
("[v2,2/5] treewide: Fix stale references of Android docs")

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 3/5] cmd: bcb: Fix duplicated handling in two case-branches

[Eugeniu Rosca]

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>
---
 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;
 	case BCB_CMD_FIELD_SET:
 		if (argc != 3)
 			goto err;
-- 
2.22.0

[U-Boot] [PATCH 3/5] cmd: bcb: Fix duplicated handling in two case-branches

[Igor Opaniuk]

On Fri, Jul 12, 2019 at 2:21 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>
> ---
>  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;
>         case BCB_CMD_FIELD_SET:
>                 if (argc != 3)
>                         goto err;
> --
> 2.22.0
>

Reviewed-by: Igor Opaniuk <igor.opaniuk@gmail.com>

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk

[U-Boot] [PATCH 3/5] cmd: bcb: Fix duplicated handling in two case-branches

[Eugeniu Rosca]

Superseded by https://patchwork.ozlabs.org/patch/1131357/
("[v2,3/5] cmd: bcb: Fix duplicated handling in two case-branches")

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 4/5] cmd: bcb: Use strcmp() instead of strncmp() for string literals

[Eugeniu Rosca]

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>
---
 cmd/bcb.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/cmd/bcb.c b/cmd/bcb.c
index 3b1c7434e287..c7138a5179a9 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,21 +85,21 @@ err:
 
 static int bcb_field_get(char *name, char **field, int *size)
 {
-	if (!strncmp(name, "command", sizeof("command"))) {
-		*field = bcb.command;
-		*size = sizeof(bcb.command);
-	} else if (!strncmp(name, "status", sizeof("status"))) {
-		*field = bcb.status;
-		*size = sizeof(bcb.status);
-	} else if (!strncmp(name, "recovery", sizeof("recovery"))) {
-		*field = bcb.recovery;
-		*size = sizeof(bcb.recovery);
-	} else if (!strncmp(name, "stage", sizeof("stage"))) {
-		*field = bcb.stage;
-		*size = sizeof(bcb.stage);
-	} else if (!strncmp(name, "reserved", sizeof("reserved"))) {
-		*field = bcb.reserved;
-		*size = sizeof(bcb.reserved);
+	if (!strcmp(name, "command")) {
+		*fieldp = bcb.command;
+		*sizep = sizeof(bcb.command);
+	} else if (!strcmp(name, "status")) {
+		*fieldp = bcb.status;
+		*sizep = sizeof(bcb.status);
+	} else if (!strcmp(name, "recovery")) {
+		*fieldp = bcb.recovery;
+		*sizep = sizeof(bcb.recovery);
+	} else if (!strcmp(name, "stage")) {
+		*fieldp = bcb.stage;
+		*sizep = sizeof(bcb.stage);
+	} else if (!strcmp(name, "reserved")) {
+		*fieldp = bcb.reserved;
+		*sizep = sizeof(bcb.reserved);
 	} else {
 		printf("Error: Unknown bcb field '%s'\n", name);
 		return -1;
-- 
2.22.0

[U-Boot] [PATCH 4/5] cmd: bcb: Use strcmp() instead of strncmp() for string literals

[Igor Opaniuk]

Hi Eugeniu,

On Fri, Jul 12, 2019 at 2:21 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>
> ---
>  cmd/bcb.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/cmd/bcb.c b/cmd/bcb.c
> index 3b1c7434e287..c7138a5179a9 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,21 +85,21 @@ err:
>
>  static int bcb_field_get(char *name, char **field, int *size)
>  {
> -       if (!strncmp(name, "command", sizeof("command"))) {
> -               *field = bcb.command;
> -               *size = sizeof(bcb.command);
> -       } else if (!strncmp(name, "status", sizeof("status"))) {
> -               *field = bcb.status;
> -               *size = sizeof(bcb.status);
> -       } else if (!strncmp(name, "recovery", sizeof("recovery"))) {
> -               *field = bcb.recovery;
> -               *size = sizeof(bcb.recovery);
> -       } else if (!strncmp(name, "stage", sizeof("stage"))) {
> -               *field = bcb.stage;
> -               *size = sizeof(bcb.stage);
> -       } else if (!strncmp(name, "reserved", sizeof("reserved"))) {
> -               *field = bcb.reserved;
> -               *size = sizeof(bcb.reserved);
> +       if (!strcmp(name, "command")) {
> +               *fieldp = bcb.command;
> +               *sizep = sizeof(bcb.command);
> +       } else if (!strcmp(name, "status")) {
> +               *fieldp = bcb.status;
> +               *sizep = sizeof(bcb.status);
> +       } else if (!strcmp(name, "recovery")) {
> +               *fieldp = bcb.recovery;
> +               *sizep = sizeof(bcb.recovery);
> +       } else if (!strcmp(name, "stage")) {
> +               *fieldp = bcb.stage;
> +               *sizep = sizeof(bcb.stage);
> +       } else if (!strcmp(name, "reserved")) {
> +               *fieldp = bcb.reserved;
> +               *sizep = sizeof(bcb.reserved);
>         } else {
>                 printf("Error: Unknown bcb field '%s'\n", name);
>                 return -1;
> --
> 2.22.0
>

Reviewed-by: Igor Opaniuk <igor.opaniuk@gmail.com>

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk

[U-Boot] [PATCH 4/5] cmd: bcb: Use strcmp() instead of strncmp() for string literals

[Eugeniu Rosca]

Hi Igor,

On Fri, Jul 12, 2019 at 02:54:19PM +0300, Igor Opaniuk wrote:
> >  static int bcb_field_get(char *name, char **field, int *size)
> >  {
> > -       if (!strncmp(name, "command", sizeof("command"))) {
> > -               *field = bcb.command;
> > +       if (!strcmp(name, "command")) {
> > +               *fieldp = bcb.command;

Many thanks for the recent reviews!

There is a small issue committed by my autopilot, i.e. I didn't do the
best partitioning of the patches, such that they could provoke a build
breakage during git bisecting (see the intermixed usage of field and
fieldp variables above). I will fix it in v2, adding your Reviewed-by
in the other patches.

Thanks again!

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 4/5] cmd: bcb: Use strcmp() instead of strncmp() for string literals

[Eugeniu Rosca]

Superseded by https://patchwork.ozlabs.org/patch/1131358/
("[v2,4/5] cmd: bcb: Use strcmp() instead of strncmp() for string literals")

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 5/5] cmd: bcb: Apply non-functional refinements

[Eugeniu Rosca]

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.

[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>
---
 cmd/bcb.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/cmd/bcb.c b/cmd/bcb.c
index c7138a5179a9..9626f2c69e34 100644
--- a/cmd/bcb.c
+++ b/cmd/bcb.c
@@ -83,7 +83,7 @@ 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")) {
 		*fieldp = bcb.command;
@@ -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

[U-Boot] [PATCH 5/5] cmd: bcb: Apply non-functional refinements

[Igor Opaniuk]

On Fri, Jul 12, 2019 at 2:26 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.

minor: could you please add more details about particular issues that
were addressed in this commit, because current commit message
contains only some insights of a glitch in the merge workflow instead of
overview of changes.

>
> [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>
> ---
>  cmd/bcb.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/cmd/bcb.c b/cmd/bcb.c
> index c7138a5179a9..9626f2c69e34 100644
> --- a/cmd/bcb.c
> +++ b/cmd/bcb.c
> @@ -83,7 +83,7 @@ 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")) {
>                 *fieldp = bcb.command;
> @@ -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
>

With my comment addressed:
Reviewed-by: Igor Opaniuk <igor.opaniuk@gmail.com>

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk

[U-Boot] [PATCH 5/5] cmd: bcb: Apply non-functional refinements

[Eugeniu Rosca]

On Fri, Jul 12, 2019 at 03:05:59PM +0300, Igor Opaniuk wrote:
> On Fri, Jul 12, 2019 at 2:26 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.
> 
> minor: could you please add more details about particular issues that
> were addressed in this commit, because current commit message
> contains only some insights of a glitch in the merge workflow instead of
> overview of changes.

Fixed in https://patchwork.ozlabs.org/patch/1131359/
("[v2,5/5] cmd: bcb: Apply non-functional refinements").

Thanks!

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 5/5] cmd: bcb: Apply non-functional refinements

[Eugeniu Rosca]

Superseded by https://patchwork.ozlabs.org/patch/1131359/
("[v2,5/5] cmd: bcb: Apply non-functional refinements")

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 0/5] Fixes and improvements in BCB and Android docs

[Eugeniu Rosca]

The whole series is obsoleted/superseded by v2:
https://patchwork.ozlabs.org/cover/1131356/
("[v2,0/5] Fixes and improvements in BCB and Android docs")

-- 
Best Regards,
Eugeniu.