[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/4] sha1sum: use AF_ALG when available
From: |
Matteo Croce |
Subject: |
Re: [PATCH v2 1/4] sha1sum: use AF_ALG when available |
Date: |
Thu, 26 Apr 2018 18:02:44 +0200 |
On Wed, Apr 25, 2018 at 9:07 PM, Paul Eggert <address@hidden> wrote:
> Thanks for working on this. Some comments:
>
Thanks for the review!
> 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.
>
Right, I get the header from md5.c which actually comes from GNU C
Library. Will remove it.
>> + 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).
>
To be compliant with the GNU coding style, I indented the code with
`indent -gnu -nut`, which removed space after sizeof. In future
iterations I'll add --blank-before-sizeof too.
Will fix in the v3
> 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;
>
I assume that C99 is safe to use in gnulib, good to know.
>> + /* 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?
>
I have to check if sendfile supports piping data from a shared memory
object, while I can't find a definition for S_TYPEISTMO in my compiler
headers (gcc 7.3.1). Is it for a typed memory object?
Anyway, the size must be known in advance to use sendfile, is the cas
for SHM and STMO?
> 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.
I agree, generating output from a library is not a good idea. I will
just return an error value.
Regards,
--
Matteo Croce
per aspera ad upstream
- [PATCH v2 1/4] sha1sum: use AF_ALG when available, (continued)
[PATCH v2 2/4] sha256sum: use kernel crypto API, Matteo Croce, 2018/04/25
[PATCH v2 3/4] sha512sum: use kernel crypto API, Matteo Croce, 2018/04/25
[PATCH v2 4/4] md5sum: use kernel crypto API, Matteo Croce, 2018/04/25