Re: [PATCH] Bindings for ‘sendfile’

[Ludovic Courtès]

Hi Mark,

Mark H Weaver <address@hidden> skribis:

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

Thanks for the quick review!

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

Yeah.  (And experience has shown that POSIX largely follows GNU/Linux
nowadays.)

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

Oops, indeed.  (Note that sendfile(2) and write(2) have that problem:
they take a size_t and return a ssize_t...)

There was also the other issue of making sure we use the right function,
depending on _FILE_OFFSET_BITS & co.

Here are the changes compared to the previous patch:

diff --git a/libguile/filesys.c b/libguile/filesys.c
index 097b03a..6804db9 100644
--- a/libguile/filesys.c
+++ b/libguile/filesys.c
@@ -102,6 +102,10 @@
 # include <sys/sendfile.h>
 #endif
 
+/* Glibc's `sendfile' function.  */
+#define sendfile_or_sendfile64                 \
+  CHOOSE_LARGEFILE (sendfile, sendfile64)
+
 #include <full-read.h>
 #include <full-write.h>
 
@@ -1123,7 +1127,7 @@ SCM_DEFINE (scm_sendfile, "sendfile", 3, 1, 0,
     }
 
   size_t c_count;
-  off_t c_offset;
+  scm_t_off c_offset;
   ssize_t result;
   int in_fd, out_fd;
 
@@ -1133,20 +1137,20 @@ SCM_DEFINE (scm_sendfile, "sendfile", 3, 1, 0,
   c_offset = SCM_UNBNDP (offset) ? 0 : scm_to_off_t (offset);
 
 #ifdef HAVE_SENDFILE
-  result = sendfile (out_fd, in_fd,
+  result = sendfile_or_sendfile64 (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.  */
+     Fall back to read(2) and write(2) when such an error occurs.  */
   if (result < 0 && errno != EINVAL && errno != ENOSYS)
     SCM_SYSERROR;
   else if (result < 0)
 #endif
   {
     char buf[8192];
-    size_t left;
+    size_t result, left;
 
     if (!SCM_UNBNDP (offset))
       {
@@ -1173,6 +1177,8 @@ SCM_DEFINE (scm_sendfile, "sendfile", 3, 1, 0,
 
        result += obtained;
       }
+
+    return scm_from_size_t (result);
   }
 
   return scm_from_ssize_t (result);

WDYT?

Thanks,
Ludo’.