>>>>> "Jean-Christophe" == Jean-Christophe PLAGNIOL-VILLARD <plagnioj@xxxxxxxxxxxx> writes:
Hi,
>> +static void print_digest(struct digest *d)
>> +{
>> + unsigned char *data;
>> + int i;
>> +
>> + data = xmalloc(d->length);
Jean-Christophe> hang here no
Jean-Christophe> please use malloc it's a command we must not hang if out of mem
Jean-Christophe> just because we use a digest command
Ok. This is cutnpaste from the crc command though. A quick look at
commands shows that others do the same:
git grep xmalloc commands/
commands/bootm.c: handle->data = xmalloc(len);
commands/cat.c: buf = xmalloc(BUFSIZE);
commands/crc.c: buf = xmalloc(4096);
commands/digest.c: data = xmalloc(d->length);
commands/digest.c: buf = xmalloc(4096);
commands/i2c.c: buf = xmalloc(count);
commands/i2c.c: buf = xmalloc(count);
commands/mem.c: rw_buf1 = xmalloc(RW_BUF_SIZE);
commands/mem.c: buf = xmalloc(RW_BUF_SIZE);
>> +static int do_digest(struct command *cmdtp, int argc, char *argv[])
>> +{
>> + char algorithm[7];
>> + struct digest *d;
>> +
>> + /* digest algoritm is command name without "sum" */
>> + strlcpy(algorithm, cmdtp->name,
>> + strstr(cmdtp->name, "sum") + 1 - cmdtp->name);
Jean-Christophe> can we do more simple?
Maybe. I wanted something automatic rather than a series of strcmp
checks, but feel free to suggest something else.
>> + d = digest_get_by_name(algorithm);
>> + BUG_ON(!d);
>> +
>> + if (argc < 2)
>> + return COMMAND_ERROR_USAGE;
>> +
>> + argv++;
>> + while (*argv) {
>> + char *filename = "/dev/mem";
>> + ulong start = 0, size = ~0;
Jean-Christophe> do we really need to declare this here?
Jean-Christophe> and /dev/mem as default
Yes, or rather you need to initialize it for each iteration of the
loop. We could move the declaration up to the beginning of the function,
but as they are only used inside the loop it imho makes more sense to
put it here.
Jean-Christophe> if yes for /dev/mem as default this should be documented in the help at least
Why? all the memory commands do that (md/mw/crc32).
>> +
>> + /* arguments are either file, file+area or area */
>> + if (parse_area_spec(*argv, &start, &size)) {
>> + filename = *argv;
>> + if (argv[1] && !parse_area_spec(argv[1], &start, &size)) {
>> + argv++;
>> + }
>> + }
>> +
>> + if (file_digest(d, filename, start, size) < 0)
>> + return 1;
Jean-Christophe> do we really need to stop if ine of them is not availlable
I don't feel strongly about it, but it seems the simplest solution.
Jean-Christophe> and we should check the getc to be able to interrupt it
crc doesn't do that either, but ok - I can add a ctrlc() check in the
main loop.
Thanks for the review.
--
Bye, Peter Korsgaard
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
