Re: `scm_c_read ()' and `swap_buffer' trick harmful

["Neil Jerram" <neiljerram@googlemail.com>]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

2008/11/20 Ludovic Courtès <ludo@gnu.org>:
>
> In theory, yes.  In practice, the notion of "unbuffered port" is
> ill-defined, I'm afraid.  The `SCM_BUF0' flag probably can't be relied
> on, as it appears to be only really used on `fports.c'.  Actually, for
> some reason (probably copy & paste), make_cbip() creates ports with
> "SCM_OPN | SCM_RDNG | SCM_BUF0", although `SCM_BUF0' is probably not
> needed.  So I think "unbuffered port" means "read_buf_size <= 1".

That's what I was thinking too.  Please see the attached.

    Neil

[-- Attachment #2: 0001-Make-scm_c_read-use-caller-buffer-only-for-unbuffere.patch --]
[-- Type: text/plain, Size: 4372 bytes --]

From 996384a935224baa2480ab02e50faa91e24b5613 Mon Sep 17 00:00:00 2001
From: Neil Jerram <neil@ossau.uklinux.net>
Date: Thu, 20 Nov 2008 21:49:35 +0000
Subject: [PATCH] Make scm_c_read use caller buffer only for unbuffered ports.

We recently modified scm_c_read so that it temporarily swaps the
caller's buffer with the port's normal read buffer, in order to
improve performance in the case where the port is unbuffered (which
actually means having a single-byte buffer) - but we implemented the
swap in the buffered case too.  The latter turns out to be a bad idea
- because it means that the C code of a custom port implementation
cannot rely on a port's buffer always being the same as when it was
first set up - and so this commit reverts that.  The buffer swapping
trick now applies to unbuffered ports only.
---
 libguile/ports.c |   72 ++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/libguile/ports.c b/libguile/ports.c
index 2b1c5e1..bfccd0b 100644
--- a/libguile/ports.c
+++ b/libguile/ports.c
@@ -1100,32 +1100,56 @@ scm_c_read (SCM port, void *buffer, size_t size)
   /* Now we will call scm_fill_input repeatedly until we have read the
      requested number of bytes.  (Note that a single scm_fill_input
      call does not guarantee to fill the whole of the port's read
-     buffer.)  For these calls, since we already have a buffer here to
-     read into, we bypass the port's own read buffer (if it has one),
-     by saving it off and modifying the port structure to point to our
-     own buffer.
-
-     We need to make sure that the port's normal buffer is reinstated
-     in case one of the scm_fill_input () calls throws an exception;
-     we use the scm_dynwind_* API to achieve that. */
-  psb.pt = pt;
-  psb.buffer = buffer;
-  psb.size = size;
-  scm_dynwind_begin (SCM_F_DYNWIND_REWINDABLE);
-  scm_dynwind_rewind_handler (swap_buffer, &psb, SCM_F_WIND_EXPLICITLY);
-  scm_dynwind_unwind_handler (swap_buffer, &psb, SCM_F_WIND_EXPLICITLY);
-
-  /* Call scm_fill_input until we have all the bytes that we need, or
-     we hit EOF. */
-  while (pt->read_buf_size && (scm_fill_input (port) != EOF))
+     buffer.) */
+  if (pt->read_buf_size <= 1)
     {
-      pt->read_buf_size -= (pt->read_end - pt->read_pos);
-      pt->read_pos = pt->read_buf = pt->read_end;
-    }
-  n_read += pt->read_buf - (unsigned char *) buffer;
+      /* The port that we are reading from is unbuffered - i.e. does
+	 not have its own persistent buffer - but we have a buffer,
+	 provided by our caller, that is the right size for the data
+	 that is wanted.  For the following scm_fill_input calls,
+	 therefore, we use the buffer in hand as the port's read
+	 buffer.
+
+	 We need to make sure that the port's normal (1 byte) buffer
+	 is reinstated in case one of the scm_fill_input () calls
+	 throws an exception; we use the scm_dynwind_* API to achieve
+	 that. */
+      psb.pt = pt;
+      psb.buffer = buffer;
+      psb.size = size;
+      scm_dynwind_begin (SCM_F_DYNWIND_REWINDABLE);
+      scm_dynwind_rewind_handler (swap_buffer, &psb, SCM_F_WIND_EXPLICITLY);
+      scm_dynwind_unwind_handler (swap_buffer, &psb, SCM_F_WIND_EXPLICITLY);
+
+      /* Call scm_fill_input until we have all the bytes that we need,
+	 or we hit EOF. */
+      while (pt->read_buf_size && (scm_fill_input (port) != EOF))
+	{
+	  pt->read_buf_size -= (pt->read_end - pt->read_pos);
+	  pt->read_pos = pt->read_buf = pt->read_end;
+	}
+      n_read += pt->read_buf - (unsigned char *) buffer;
 
-  /* Reinstate the port's normal buffer. */
-  scm_dynwind_end ();
+      /* Reinstate the port's normal buffer. */
+      scm_dynwind_end ();
+    }
+  else
+    {
+      /* The port has its own buffer.  It is important that we use it,
+	 even if it happens to be smaller than our caller's buffer, so
+	 that a custom port implementation's entry points (in
+	 particular, fill_input) can rely on the buffer always being
+	 the same as they first set up. */
+      while (size && (scm_fill_input (port) != EOF))
+	{
+	  n_available = min (size, pt->read_end - pt->read_pos);
+	  memcpy (buffer, pt->read_pos, n_available);
+	  buffer = (char *) buffer + n_available;
+	  pt->read_pos += n_available;
+	  n_read += n_available;
+	  size -= n_available;
+	} 
+    }
 
   return n_read;
 }
-- 
1.5.6.5