[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/4] sha1sum: use AF_ALG when available
From: |
Paul Eggert |
Subject: |
Re: [PATCH v2 1/4] sha1sum: use AF_ALG when available |
Date: |
Wed, 25 Apr 2018 12:07:27 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
Thanks for working on this. Some comments:
On 04/25/2018 04:26 AM, Matteo Croce wrote:
+ This file is part of the GNU C Library.
Is it really part of glibc? If not, please remove that comment.
+ if (strlen (alg) >= sizeof(salg.salg_name))
+ return -EINVAL;
...
+ strcpy ((char *) salg.salg_name, alg);
Space before paren is the usual glibc and gnulib style, for both
functions and sizeof. Please check your code for this elsewhere, as
there are other instances.
Better, sizeof need not use parens when its argument is an lvalue, as is
the case here (and elsewhere).
Better yet, for this particular case, copy and check at the same time so
that your algorithm doesn't have unbounded CPU time when alg is very
long, plus this avoids a cast. Something like this:
for (int i = 0; (salg.salg_name[i] = alg[i]); i++)
if (i == sizeof salg.salg_name - 1)
return -EINVAL;
+ /* if file is a regular file, attempt sendfile() to pipe the data */
+ if (!fstat (fileno (stream), &st) && S_ISREG (st.st_mode) &&
+ st.st_size <= MAX_RW_COUNT)
The comment should end with "data. */" with two spaces after the
period. Similarly for other comments.
"&&" should be at the beginning of the next line, not at the end of the
previous one.
POSIX says st_size is also valid if S_TYPEISSHM (&st) || S_TYPEISTMO
(&st); perhaps test for that too?
Even if st_size is greater than MAX_RW_COUNT, the code can still use
sendfile in a loop; why not do that and get rid of the MAX_RW_COUNT
here? Surely that would be more efficient for large files.
+ fprintf (stderr, "Error from read (%zd vs %zd bytes): %s\n",
+ size, hashlen, strerror (errno));
This is not right; this low-level function should not output to stderr
when there is a read error. Instead, it should simply return an error
value, as it does when there is a write error.