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

[Matteo Croce]

On Mon, Apr 23, 2018 at 5:07 PM, Tim Rühsen <address@hidden> wrote:
>
> On 04/23/2018 01:17 PM, Matteo Croce wrote:
>> +#include <config.h>
>> +
>> +#include "af_alg.h"
>> +
>> +/* from linux/include/linux/fs.h: (INT_MAX & PAGE_MASK) */
>> +#define MAX_RW_COUNT 0x7FFFF000
>> +#define BLOCKSIZE 32768
>> +
>> +int
>> +afalg_stream (FILE * stream, void *resblock, const char *alg, int hashlen)
>> +{
>> +  struct sockaddr_alg salg = {
>> +    .salg_family = AF_ALG,
>> +    .salg_type = "hash",
>> +  };
>> +  int ret, cfd, ofd;
>> +  static char buf[BLOCKSIZE];
>> +  ssize_t size;
>> +  struct stat st;
>> +
>> +  strcpy((char *)salg.salg_name, alg);
>
> Better consider for size of salg.salg_name here.
>

Right. Will check it in v2.

>> +  cfd = socket (AF_ALG, SOCK_SEQPACKET, 0);
>> +  if (cfd < 0)
>> +      return -EAFNOSUPPORT;
>
> What about moving salg initialization here ?
>

I think that contextual initialization it's a good trick to fill it
with zeroes avoiding a memset later.
What are the advantage of initializing the structure later? The effort
should be minimal in case of no AF_ALG support.

>> +  ret = bind (cfd, (struct sockaddr *) &salg, sizeof (salg));
>> +  if (ret < 0)
>> +    {
>> +      ret = -EAFNOSUPPORT;
>> +      goto out_cfd;
>> +    }
>> +
>> +  ofd = accept (cfd, NULL, 0);
>> +  if (ofd < 0)
>> +    {
>> +      ret = -EAFNOSUPPORT;
>> +      goto out_ofd;
>> +    }
>> +
>> +  /* 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)
>
> Is it possible to skip that MAX_RW_COUNT check on recent glibc ?
> From 'man sendfile':
> "
> The original Linux sendfile() system call was not designed to handle
> large file offsets.  Consequently, Linux 2.4 added
> sendfile64(), with a wider type for the offset argument.  The glibc
> sendfile()  wrapper  function  transparently  deals
> with the kernel differences.
> "
>

The advantage of sendfile64() over sendfile() is only that the offset
in the input file can be bigger.
I've tried it and it seems that MAX_RW_COUNT is an hard limit on every system:

$ strace -e read dd if=/dev/zero of=/dev/null bs=3G count=1
read(0, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
3221225472) = 2147479552
0+1 records in
0+1 records out
2147479552 bytes (2.1 GB, 2.0 GiB) copied, 0.221906 s, 9.7 GB/s

As reported in the manpage too:

"sendfile() will transfer at most 0x7ffff000 (2,147,479,552) bytes,
returning the number of bytes actually transferred.  (This is true on
both 32-bit and 64-bit systems.)"

>> +    {
>> +      if (sendfile(ofd, fileno(stream), NULL, st.st_size) == -1)
>
> From 'man sendfile':
> "
> Note  that  a  successful  call  to
> sendfile()  may  write fewer bytes than requested; the caller should be
> prepared to retry the call if there were unsent
> bytes.  See also NOTES.
> "
>
>> +        ret = -EIO;
>> +      else
>> +        ret = 0;
>> +    } else {
>> +      /* sendfile() not possible, do a classic read-write loop */
>> +      while ((size = fread (buf, 1, sizeof (buf), stream)))
>> +        {
>> +          if (send (ofd, buf, size, size == sizeof (buf) ? MSG_MORE : 0) == 
>> -1)
>> +            {
>> +              ret = -EIO;
>> +              goto out_ofd;
>> +            }
>> +        }
>> +  }
>

I might be wrong here, but looking at the kernel code of the
sendfile64(), which actually calls do_splice_direct() that calls
splice_direct_to_actor(),
it can happen only with nonblocking sockets. I didn't managed to
trigger such behaviour myself, but more testing is welcome.

>
> Regards, Tim
>

Regards,
-- 
Matteo Croce
per aspera ad upstream