[PATCH 2/4] cmd: dtimg: Merge duplicated prints

[Sam Protsenko]

Hi,

On Fri, Nov 29, 2019 at 9:30 PM Eugeniu Rosca <erosca at de.adit-jv.com> wrote:
>
> Getting DTB/DTBO header address happens twice (in do_dtimg_dump and
> in dtimg_get_fdt) with duplicating below error messages:
>  - Error: Wrong image address
>  - Error: DT image header is incorrect
>

Actually, I intentionally left it that way for the sake of code
simplicity. Didn't want to introduce one more function: more lines of
code, more entities, doesn't solve any issues in this particular case,
as I see it. Speaking of which, have you tried to compare how the
binary footprint changes (.text, .data. .rodata, using buildman) when
you apply this patch? I have a feeling those duplicated strings are in
fact optimized out by compiler. Relied on that fact actually. As they
say: all problems in computer science can be solved by another level
of indirection... except for the problem of too many layers of
indirection :)

In one word: if you feel it's better, I don't mind:

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

More detailed review is below.

> Reduce the duplication and improve the error message by appending
> the faulty address value:
>  - Error: Wrong image address '0x48000000z'
>
> Signed-off-by: Eugeniu Rosca <erosca at de.adit-jv.com>
> ---
>  cmd/dtimg.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/cmd/dtimg.c b/cmd/dtimg.c
> index 2317c859953d..5989081b0c14 100644
> --- a/cmd/dtimg.c
> +++ b/cmd/dtimg.c
> @@ -13,18 +13,13 @@ enum cmd_dtimg_info {
>         CMD_DTIMG_SIZE,
>  };
>
> -static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
> -                        char * const argv[])
> +static int dtimg_get_argv_addr(char * const str, ulong *hdr_addrp)
>  {
>         char *endp;
> -       ulong hdr_addr;
> +       ulong hdr_addr = simple_strtoul(str, &endp, 16);
>
> -       if (argc != 2)
> -               return CMD_RET_USAGE;
> -
> -       hdr_addr = simple_strtoul(argv[1], &endp, 16);
>         if (*endp != '\0') {
> -               printf("Error: Wrong image address\n");
> +               printf("Error: Wrong image address '%s'\n", str);
>                 return CMD_RET_FAILURE;

As now this function is not a command handler anymore, but just an
internal function, I suggest using 'bool' as its return type, and
return true/false instead of CMD_RET_* (because further you're
checking its return value anyway, it might be prettier this way).

>         }
>
> @@ -32,6 +27,21 @@ static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
>                 printf("Error: DT image header is incorrect\n");
>                 return CMD_RET_FAILURE;
>         }
> +       *hdr_addrp = hdr_addr;
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
> +static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
> +                        char * const argv[])
> +{
> +       ulong hdr_addr;
> +
> +       if (argc != 2)
> +               return CMD_RET_USAGE;
> +
> +       if (dtimg_get_argv_addr(argv[1], &hdr_addr) != CMD_RET_SUCCESS)
> +               return CMD_RET_FAILURE;
>
>         android_dt_print_contents(hdr_addr);
>
> @@ -50,16 +60,8 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
>         if (argc != 4)
>                 return CMD_RET_USAGE;
>
> -       hdr_addr = simple_strtoul(argv[1], &endp, 16);
> -       if (*endp != '\0') {
> -               printf("Error: Wrong image address\n");
> +       if (dtimg_get_argv_addr(argv[1], &hdr_addr) != CMD_RET_SUCCESS)
>                 return CMD_RET_FAILURE;
> -       }
> -
> -       if (!android_dt_check_header(hdr_addr)) {
> -               printf("Error: DT image header is incorrect\n");
> -               return CMD_RET_FAILURE;
> -       }
>
>         index = simple_strtoul(argv[2], &endp, 0);
>         if (*endp != '\0') {
> --
> 2.24.0
>