On Wed, Apr 25, 2018 at 9:07 PM, Paul Eggert <egg...@cs.ucla.edu> 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
