bug-gnulib
[Top][All Lists]
Advanced

[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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]