Re: [PATCH] Bindings for ‘sendfile’

[Mark H Weaver]

Hi Ludovic,

address@hidden (Ludovic Courtès) writes:
> I plan to commit the patch below, which adds bindings for ‘sendfile’.
>
> Comments?

Looks great to me, modulo one comment below.

I especially like the fact that although it can make use of the
non-standard Linux syscall, it works properly on all systems.  In
response to suggestions by others that we create a "linux" module:
I'd prefer to follow the good precedent set by Ludovic here.

> diff --git a/libguile/filesys.c b/libguile/filesys.c
> index 282ff31..097b03a 100644
> --- a/libguile/filesys.c
> +++ b/libguile/filesys.c
> @@ -98,6 +98,14 @@
>  
>  #define NAMLEN(dirent)  strlen ((dirent)->d_name)
>  
> +#ifdef HAVE_SYS_SENDFILE_H
> +# include <sys/sendfile.h>
> +#endif
> +
> +#include <full-read.h>
> +#include <full-write.h>

I didn't know about Gnulib's 'full-read' and 'full-write'.  Nice!
We should probably make more use of these throughout the tree.

> +SCM_DEFINE (scm_sendfile, "sendfile", 3, 1, 0,
> +         (SCM out, SCM in, SCM count, SCM offset),
> +         "Send @var{count} bytes from @var{in} to @var{out}, both of which "
> +         "are either open file ports or file descriptors.  When "
> +         "@var{offset} is omitted, start reading from @var{in}'s current "
> +         "position; otherwise, start reading at @var{offset}.")
> +#define FUNC_NAME s_scm_sendfile
> +{
> +#define VALIDATE_FD_OR_PORT(cvar, svar, pos) \
> +  if (scm_is_integer (svar))                 \
> +    cvar = scm_to_int (svar);                        \
> +  else                                               \
> +    {                                                \
> +      SCM_VALIDATE_OPFPORT (pos, svar);              \
> +      scm_flush (svar);                              \
> +      cvar = SCM_FPORT_FDES (svar);          \
> +    }
> +
> +  size_t c_count;
> +  off_t c_offset;
> +  ssize_t result;
> +  int in_fd, out_fd;
> +
> +  VALIDATE_FD_OR_PORT (out_fd, out, 1);
> +  VALIDATE_FD_OR_PORT (in_fd, in, 2);
> +  c_count = scm_to_size_t (count);

Since the code below will behave badly if 'c_count' does not fit in an
'ssize_t', we should validate here that it _does_ fit.

> +  c_offset = SCM_UNBNDP (offset) ? 0 : scm_to_off_t (offset);
> +
> +#ifdef HAVE_SENDFILE
> +  result = sendfile (out_fd, in_fd,
> +                  SCM_UNBNDP (offset) ? NULL : &c_offset,
> +                  c_count);
> +
> +  /* Quoting the Linux man page: "In Linux kernels before 2.6.33, out_fd
> +     must refer to a socket.  Since Linux 2.6.33 it can be any file."
> +     Fall back to read(2) and write(2) such an error happens.  */
> +  if (result < 0 && errno != EINVAL && errno != ENOSYS)
> +    SCM_SYSERROR;
> +  else if (result < 0)
> +#endif
> +  {
> +    char buf[8192];
> +    size_t left;
> +
> +    if (!SCM_UNBNDP (offset))
> +      {
> +     if (SCM_PORTP (in))
> +       scm_seek (in, offset, scm_from_int (SEEK_SET));
> +     else
> +       lseek_or_lseek64 (in_fd, c_offset, SEEK_SET);
> +      }
> +
> +    for (result = 0, left = c_count; result < c_count; )

If 'c_count's does not fit in a 'ssize_t', then this loop will go
forever and 'result' will wrap around to negative numbers and undefined
C behavior.

> +      {
> +     size_t asked, obtained;
> +
> +     asked = SCM_MIN (sizeof buf, left);
> +     obtained = full_read (in_fd, buf, asked);
> +     if (obtained < asked)
> +       SCM_SYSERROR;
> +
> +     left -= obtained;
> +
> +     obtained = full_write (out_fd, buf, asked);
> +     if (obtained < asked)
> +       SCM_SYSERROR;
> +
> +     result += obtained;
> +      }
> +  }
> +
> +  return scm_from_ssize_t (result);
> +
> +#undef VALIDATE_FD_OR_PORT
> +}
> +#undef FUNC_NAME
> +
>  #endif /* HAVE_POSIX */

Other than that, looks beautiful to me :)

    Thanks!
      Mark