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

Hi,

On Fri, Nov 29, 2019 at 9:30 PM Eugeniu Rosca erosca@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@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@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