Hi Nikolay, On 16 December 2014 at 15:29, Nikolay Dimitrov <picmas...@mail.bg> wrote: > Hi Simon, > > I omitted one clarification, which I think it's important. > > > On 12/17/2014 12:25 AM, Nikolay Dimitrov wrote: >> >> Hi Simon, >> >> On 12/17/2014 12:02 AM, Simon Glass wrote: >>> >>> Hi Nikolay, >>> >>> On 12 December 2014 at 11:01, <picmas...@mail.bg> wrote: >>>> >>>> From: Nikolay Dimitrov <picmas...@mail.bg> >>>> >>>> Fix issue in parse_verify_sum() which swaps handling of env-var and >>>> *address. >>>> Move hash_command() argc check earlier. >>>> Cosmetic change on do_hash() variable declaration. >>>> Improved help message for "hash" command. >>>> >>>> Signed-off-by: Nikolay Dimitrov <picmas...@mail.bg> >>> >>> >>> Thanks for this. Main change looks good, a few nits. >>> >>>> --- >>>> common/cmd_hash.c | 28 +++++++++++++--------------- >>>> common/hash.c | 6 ++---- >>>> 2 files changed, 15 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/common/cmd_hash.c b/common/cmd_hash.c >>>> index 90facbb..704d21e 100644 >>>> --- a/common/cmd_hash.c >>>> +++ b/common/cmd_hash.c >>>> @@ -18,9 +18,9 @@ >>>> static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * >>>> const argv[]) >>>> { >>>> char *s; >>>> -#ifdef CONFIG_HASH_VERIFY >>>> int flags = HASH_FLAG_ENV; >>>> >>>> +#ifdef CONFIG_HASH_VERIFY >>>> if (argc < 4) >>>> return CMD_RET_USAGE; >>>> if (!strcmp(argv[1], "-v")) { >>>> @@ -28,8 +28,6 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int >>>> argc, char * const argv[]) >>>> argc--; >>>> argv++; >>>> } >>>> -#else >>>> - const int flags = HASH_FLAG_ENV; >>>> #endif >>>> /* Move forward to 'algorithm' parameter */ >>>> argc--; >>>> @@ -40,19 +38,19 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, >>>> int argc, char * const argv[]) >>>> } >>>> >>>> #ifdef CONFIG_HASH_VERIFY >>>> -U_BOOT_CMD( >>>> - hash, 6, 1, do_hash, >>>> - "compute hash message digest", >>>> - "algorithm address count [[*]sum_dest]\n" >>>> - " - compute message digest [save to env var / >>>> *address]\n" >>>> - "hash -v algorithm address count [*]sum\n" >>>> - " - verify hash of memory area with env var / >>>> *address" >>>> -); >>>> +#define HARGS 6 >>>> #else >>>> +#define HARGS 5 >>>> +#endif >>>> + >>>> U_BOOT_CMD( >>>> - hash, 5, 1, do_hash, >>>> - "compute message digest", >>>> - "algorithm address count [[*]sum_dest]\n" >>>> + hash, HARGS, 1, do_hash, >>>> + "compute hash message digest", >>>> + "algorithm address count [[*]hash_dest]\n" >>>> " - compute message digest [save to env var / >>>> *address]" >>>> -); >>>> +#ifdef CONFIG_HASH_VERIFY >>>> + "\nhash -v algorithm address count [*]hash\n" >>>> + " - verify message digest of memory area to >>>> immediate value, \n" >>> >>> >>> Perhaps " verify message digest of memory area, display result or >>> write to env var or *address" >> >> >> I think that the initial description was appropriate, because the >> "hash" command supports 3 modes of operation (I chose crc32 for shorter >> examples): >> >> 1. Verify hash against immediate value, like this >> >> hash -v crc32 0x20000000 $filesize e9b11acd >> >> 2. Verify hash against environment variable, which holds the actual >> hash value >> >> hash -v crc32 0x20000000 $filesize env_var_name >> >> 3, Verify hash against value, placed at specific memory address >> >> hash -v crc32 0x20000000 $filesize *0x30000000 >> >> As far as I observed the code and its behavior, the only case when the >> "hash -v" result was printed was when the verification failed. Please >> correct me if I'm wrong. > > > In all these modes of verification, the last argument is used as the > "reference value" to compare against, this is another reason why it > would be inappropriate to say "...or write to env var or *address". > > >> >>>> #endif >>>> +); >>>> diff --git a/common/hash.c b/common/hash.c >>>> index 12d6759..aceabc5 100644 >>>> --- a/common/hash.c >>>> +++ b/common/hash.c >>>> @@ -256,7 +256,7 @@ static int parse_verify_sum(struct hash_algo >>>> *algo, char *verify_str, >>>> env_var = 1; >>>> } >>>> >>>> - if (env_var) { >>>> + if (!env_var) { >>>> ulong addr; >>>> void *buf; >>>> >>>> @@ -347,7 +347,7 @@ int hash_command(const char *algo_name, int >>>> flags, cmd_tbl_t *cmdtp, int flag, >>>> { >>>> ulong addr, len; >>>> >>>> - if (argc < 2) >>>> + if ((argc < 2) || ((flags & HASH_FLAG_VERIFY) && (argc < 3))) >>>> return CMD_RET_USAGE; >>>> >>>> addr = simple_strtoul(*argv++, NULL, 16); >>>> @@ -380,8 +380,6 @@ int hash_command(const char *algo_name, int >>>> flags, cmd_tbl_t *cmdtp, int flag, >>>> #else >>>> if (0) { >>>> #endif >>>> - if (!argc) >>>> - return CMD_RET_USAGE; >>> >>> >>> What does this change achieve? >> >> >> I moved the verification earlier, because the original implementation >> first calculated the hash and just then complained about the last >> missing argument. My personal understanding is that errors should be >> caught as early as possible, and work should be done only as necessary >> :D. >> >>> >>>> if (parse_verify_sum(algo, *argv, vsum, >>>> flags & HASH_FLAG_ENV)) { >>>> printf("ERROR: %s does not contain a >>>> valid " >>>> -- >>>> 1.7.10.4
Thanks for the explanation, it looks right to me. Reviewed-by: Simon Glass <s...@chromium.org> Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot