Re: [PATCH v2 1/4] sha1sum: use AF_ALG when available

[Matteo Croce]

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