From: Tony Cook <tony@develop-help.com>
Date: Sat, 10 Sep 2011 04:00:10 +0000 (+1000)
Subject: remove buffering from the callback IO object implementation
X-Git-Tag: v_iobuf~28
X-Git-Url: http://git.imager.perl.org/imager.git/commitdiff_plain/8c2fe37a2612a261cc53bcb955be11bd31974770

remove buffering from the callback IO object implementation

   - the callback IO object did its own buffering, controlled by the
     maxbuffer parameter supplied to the read() and write() methods.
     This buffering has been removed, to avoid redundancy with the
     common io_glue buffering.

   - the callback IO object new tests the result of calling the close
     callback, which should return true for success.
---

diff --git a/Changes b/Changes
index 68f62ba3..1cfe41c1 100644
--- a/Changes
+++ b/Changes
@@ -15,6 +15,14 @@ Imager release history.  Older releases can be found in Changes.old
    - TIFF now uses wrapper functions of the correct types to avoid casts
      https://rt.cpan.org/Ticket/Display.html?id=69912
 
+   - the callback IO object did its own buffering, controlled by the
+     maxbuffer parameter supplied to the read() and write() methods.
+     This buffering has been removed, to avoid redundancy with the
+     common io_glue buffering.
+
+   - the callback IO object new tests the result of calling the close
+     callback, which should return true for success.
+
 To do:
 
  - make sure i_io_close() is called and the result is checked
@@ -25,6 +33,14 @@ To do:
 
  - coverage/functional tests for the buffering code
 
+ - document the new Imager::IO methods
+
+ - document the io_glue C callbacks properly
+
+ - document the Imager::io_new_cb() callback functions properly.
+
+ - fix buffered seeks
+
 Imager 0.86
 ===========
 
diff --git a/Imager.xs b/Imager.xs
index 1b638177..1a8672c8 100644
--- a/Imager.xs
+++ b/Imager.xs
@@ -143,73 +143,11 @@ struct cbdata {
   SV *readcb;
   SV *seekcb;
   SV *closecb;
-
-  /* we need to remember whether the buffer contains write data or 
-     read data
-   */
-  int reading;
-  int writing;
-
-  /* how far we've read into the buffer (not used for writing) */
-  int where;
-
-  /* the amount of space used/data available in the buffer */
-  int used;
-
-  /* the maximum amount to fill the buffer before flushing
-     If any write is larger than this then the buffer is flushed and 
-     the full write is performed.  The write is _not_ split into 
-     maxwrite sized calls
-   */
-  int maxlength;
-
-  char buffer[CBDATA_BUFSIZE];
 };
 
-/* 
-
-call_writer(cbd, buf, size)
-
-Low-level function to call the perl writer callback.
-
-*/
-
-static ssize_t call_writer(struct cbdata *cbd, void const *buf, size_t size) {
-  dTHX;
-  int count;
-  int success;
-  SV *sv;
-  dSP;
-
-  if (!SvOK(cbd->writecb))
-    return -1;
-
-  ENTER;
-  SAVETMPS;
-  EXTEND(SP, 1);
-  PUSHMARK(SP);
-  PUSHs(sv_2mortal(newSVpv((char *)buf, size)));
-  PUTBACK;
-
-  count = perl_call_sv(cbd->writecb, G_SCALAR);
-
-  SPAGAIN;
-  if (count != 1)
-    croak("Result of perl_call_sv(..., G_SCALAR) != 1");
-
-  sv = POPs;
-  success = SvTRUE(sv);
-
-
-  PUTBACK;
-  FREETMPS;
-  LEAVE;
-
-  return success ? size : -1;
-}
-
-static ssize_t call_reader(struct cbdata *cbd, void *buf, size_t size, 
-                           size_t maxread) {
+static ssize_t
+call_reader(struct cbdata *cbd, void *buf, size_t size, 
+            size_t maxread) {
   dTHX;
   int count;
   int result;
@@ -257,21 +195,8 @@ static ssize_t call_reader(struct cbdata *cbd, void *buf, size_t size,
   return result;
 }
 
-static ssize_t write_flush(struct cbdata *cbd) {
-  dTHX;
-  ssize_t result;
-
-  if (cbd->used) {
-    result = call_writer(cbd, cbd->buffer, cbd->used);
-    cbd->used = 0;
-    return result;
-  }
-  else {
-    return 1; /* success of some sort */
-  }
-}
-
-static off_t io_seeker(void *p, off_t offset, int whence) {
+static off_t
+io_seeker(void *p, off_t offset, int whence) {
   dTHX;
   struct cbdata *cbd = p;
   int count;
@@ -281,17 +206,6 @@ static off_t io_seeker(void *p, off_t offset, int whence) {
   if (!SvOK(cbd->seekcb))
     return -1;
 
-  if (cbd->writing) {
-    if (cbd->used && write_flush(cbd) <= 0)
-      return -1;
-    cbd->writing = 0;
-  }
-  if (whence == SEEK_CUR && cbd->reading && cbd->where != cbd->used) {
-    offset -= cbd->where - cbd->used;
-  }
-  cbd->reading = 0;
-  cbd->where = cbd->used = 0;
-  
   ENTER;
   SAVETMPS;
   EXTEND(SP, 2);
@@ -316,95 +230,57 @@ static off_t io_seeker(void *p, off_t offset, int whence) {
   return result;
 }
 
-static ssize_t io_writer(void *p, void const *data, size_t size) {
+static ssize_t
+io_writer(void *p, void const *data, size_t size) {
   dTHX;
   struct cbdata *cbd = p;
+  I32 count;
+  SV *sv;
+  dSP;
+  bool success;
 
-  /* printf("io_writer(%p, %p, %u)\n", p, data, size); */
-  if (!cbd->writing) {
-    if (cbd->reading && cbd->where < cbd->used) {
-      /* we read past the place where the caller expected us to be
-         so adjust our position a bit */
-      if (io_seeker(p, cbd->where - cbd->used, SEEK_CUR) < 0) {
-        return -1;
-      }
-      cbd->reading = 0;
-    }
-    cbd->where = cbd->used = 0;
-  }
-  cbd->writing = 1;
-  if (cbd->used && cbd->used + size > cbd->maxlength) {
-    int write_res = write_flush(cbd);
-    if (write_res <= 0) {
-      return write_res;
-    }
-    cbd->used = 0;
-  }
-  if (cbd->used+size <= cbd->maxlength) {
-    memcpy(cbd->buffer + cbd->used, data, size);
-    cbd->used += size;
-    return size;
-  }
-  /* it doesn't fit - just pass it up */
-  return call_writer(cbd, data, size);
+  if (!SvOK(cbd->writecb))
+    return -1;
+
+  ENTER;
+  SAVETMPS;
+  EXTEND(SP, 1);
+  PUSHMARK(SP);
+  PUSHs(sv_2mortal(newSVpv((char *)data, size)));
+  PUTBACK;
+
+  count = perl_call_sv(cbd->writecb, G_SCALAR);
+
+  SPAGAIN;
+  if (count != 1)
+    croak("Result of perl_call_sv(..., G_SCALAR) != 1");
+
+  sv = POPs;
+  success = SvTRUE(sv);
+
+
+  PUTBACK;
+  FREETMPS;
+  LEAVE;
+
+  return success ? size : -1;
 }
 
 static ssize_t 
 io_reader(void *p, void *data, size_t size) {
   dTHX;
   struct cbdata *cbd = p;
-  ssize_t total;
+  ssize_t total = 0;
   char *out = data; /* so we can do pointer arithmetic */
 
-  /* printf("io_reader(%p, %p, %d)\n", p, data, size); */
-  if (cbd->writing) {
-    if (write_flush(cbd) <= 0)
-      return 0;
-    cbd->writing = 0;
-  }
-
-  cbd->reading = 1;
-  if (size <= cbd->used - cbd->where) {
-    /* simplest case */
-    memcpy(data, cbd->buffer+cbd->where, size);
-    cbd->where += size;
-    return size;
-  }
-  total = 0;
-  memcpy(out, cbd->buffer + cbd->where, cbd->used - cbd->where);
-  total += cbd->used - cbd->where;
-  size  -= cbd->used - cbd->where;
-  out   += cbd->used - cbd->where;
-  if (size < sizeof(cbd->buffer)) {
-    int did_read = 0;
-    int copy_size;
-    while (size
-	   && (did_read = call_reader(cbd, cbd->buffer, size, 
-				    sizeof(cbd->buffer))) > 0) {
-      cbd->where = 0;
-      cbd->used  = did_read;
-
-      copy_size = i_min(size, cbd->used);
-      memcpy(out, cbd->buffer, copy_size);
-      cbd->where += copy_size;
-      out   += copy_size;
-      total += copy_size;
-      size  -= copy_size;
-    }
-    if (did_read < 0)
-      return -1;
-  }
-  else {
-    /* just read the rest - too big for our buffer*/
-    int did_read;
-    while (size > 0 && (did_read = call_reader(cbd, out, size, size)) > 0) {
-      size  -= did_read;
-      total += did_read;
-      out   += did_read;
-    }
-    if (did_read < 0)
-      return -1;
+  int did_read;
+  while (size > 0 && (did_read = call_reader(cbd, out, size, size)) > 0) {
+    size  -= did_read;
+    total += did_read;
+    out   += did_read;
   }
+  if (total == 0 && did_read < 0)
+    return -1;
 
   return total;
 }
@@ -412,30 +288,31 @@ io_reader(void *p, void *data, size_t size) {
 static int io_closer(void *p) {
   dTHX;
   struct cbdata *cbd = p;
-
-  if (cbd->writing && cbd->used > 0) {
-    if (write_flush(cbd) < 0)
-      return -1;
-    cbd->writing = 0;
-  }
+  int success = 1;
 
   if (SvOK(cbd->closecb)) {
     dSP;
+    I32 count;
+    SV *sv;
 
     ENTER;
     SAVETMPS;
     PUSHMARK(SP);
     PUTBACK;
 
-    perl_call_sv(cbd->closecb, G_VOID);
+    count = perl_call_sv(cbd->closecb, G_SCALAR);
 
     SPAGAIN;
+    
+    sv = POPs;
+    success = SvTRUE(sv);
+
     PUTBACK;
     FREETMPS;
     LEAVE;
   }
 
-  return 0;
+  return success ? 0 : -1;
 }
 
 static void io_destroyer(void *p) {
@@ -1079,10 +956,6 @@ io_new_cb(writecb, readcb, seekcb, closecb, maxwrite = CBDATA_BUFSIZE)
         cbd->seekcb = seekcb;
         SvREFCNT_inc(closecb);
         cbd->closecb = closecb;
-        cbd->reading = cbd->writing = cbd->where = cbd->used = 0;
-        if (maxwrite > CBDATA_BUFSIZE)
-          maxwrite = CBDATA_BUFSIZE;
-        cbd->maxlength = maxwrite;
         RETVAL = io_new_cb(cbd, io_reader, io_writer, io_seeker, io_closer, 
                            io_destroyer);
       OUTPUT:
diff --git a/iolayer.c b/iolayer.c
index 9e360dc0..12c4419f 100644
--- a/iolayer.c
+++ b/iolayer.c
@@ -203,8 +203,8 @@ realseek_read(io_glue *igo, void *buf, size_t count) {
   size_t        bc = 0;
   char       *cbuf = buf;
 
-  IOL_DEB( fprintf(IOL_DEBs, "realseek_read:  buf = %p, count = %d\n", 
-		  buf, count) );
+  IOL_DEB( fprintf(IOL_DEBs, "realseek_read:  buf = %p, count = %u\n", 
+		   buf, (unsigned)count) );
   /* Is this a good idea? Would it be better to handle differently?
      skip handling? */
   while( count!=bc && (rc = ig->readcb(p,cbuf+bc,count-bc))>0 ) {
@@ -212,7 +212,7 @@ realseek_read(io_glue *igo, void *buf, size_t count) {
   }
   
   ier->cpos += bc;
-  IOL_DEB( fprintf(IOL_DEBs, "realseek_read: rc = %d, bc = %d\n", rc, bc) );
+  IOL_DEB( fprintf(IOL_DEBs, "realseek_read: rc = %d, bc = %u\n", (int)rc, (unsigned)bc) );
   return rc < 0 ? rc : bc;
 }
 
@@ -240,7 +240,7 @@ realseek_write(io_glue *igo, const void *buf, size_t count) {
   char       *cbuf = (char*)buf; 
   
   IOL_DEB( fprintf(IOL_DEBs, "realseek_write: ig = %p, ier->cpos = %ld, buf = %p, "
-                  "count = %d\n", ig, (long) ier->cpos, buf, count) );
+		   "count = %u\n", ig, (long) ier->cpos, buf, (unsigned)count) );
 
   /* Is this a good idea? Would it be better to handle differently? 
      skip handling? */
@@ -249,7 +249,7 @@ realseek_write(io_glue *igo, const void *buf, size_t count) {
   }
 
   ier->cpos += bc;
-  IOL_DEB( fprintf(IOL_DEBs, "realseek_write: rc = %d, bc = %d\n", rc, bc) );
+  IOL_DEB( fprintf(IOL_DEBs, "realseek_write: rc = %d, bc = %u\n", (int)rc, (unsigned)bc) );
   return rc < 0 ? rc : bc;
 }
 
@@ -339,7 +339,7 @@ buffer_read(io_glue *igo, void *buf, size_t count) {
   io_buffer *ig = (io_buffer *)igo;
   io_ex_buffer *ieb = igo->exdata;
 
-  IOL_DEB( fprintf(IOL_DEBs, "buffer_read: ieb->cpos = %ld, buf = %p, count = %d\n", (long) ieb->cpos, buf, count) );
+  IOL_DEB( fprintf(IOL_DEBs, "buffer_read: ieb->cpos = %ld, buf = %p, count = %u\n", (long) ieb->cpos, buf, (unsigned)count) );
 
   if ( ieb->cpos+count > ig->len ) {
     mm_log((1,"buffer_read: short read: cpos=%ld, len=%ld, count=%ld\n", (long)ieb->cpos, (long)ig->len, (long)count));
@@ -1500,7 +1500,6 @@ i_io_flush(io_glue *ig) {
 
 int
 i_io_close(io_glue *ig) {
-  int flush_res = 0;
   int result = 0;
 
   IOL_DEB(fprintf(IOL_DEBs, "i_io_close(%p)\n", ig));
@@ -1607,10 +1606,10 @@ i_io_dump(io_glue *ig, int flags) {
   fprintf(IOL_DEBs, "  exdata: %p\n", ig->exdata);
   if (flags & I_IO_DUMP_CALLBACKS) {
     fprintf(IOL_DEBs, "  readcb: %p\n", ig->readcb);
-    fprintf(IOL_DEBs, "  writecb: %d\n", ig->writecb);
-    fprintf(IOL_DEBs, "  seekcb: %d\n", ig->seekcb);
-    fprintf(IOL_DEBs, "  closecb: %d\n", ig->closecb);
-    fprintf(IOL_DEBs, "  sizecb: %d\n", ig->sizecb);
+    fprintf(IOL_DEBs, "  writecb: %p\n", ig->writecb);
+    fprintf(IOL_DEBs, "  seekcb: %p\n", ig->seekcb);
+    fprintf(IOL_DEBs, "  closecb: %p\n", ig->closecb);
+    fprintf(IOL_DEBs, "  sizecb: %p\n", ig->sizecb);
   }
   if (flags & I_IO_DUMP_BUFFER) {
     fprintf(IOL_DEBs, "  buffer: %p\n", ig->buffer);
diff --git a/t/t07iolayer.t b/t/t07iolayer.t
index da792d68..2aa0d6d2 100644
--- a/t/t07iolayer.t
+++ b/t/t07iolayer.t
@@ -1,6 +1,6 @@
 #!perl -w
 use strict;
-use Test::More tests => 74;
+use Test::More tests => 76;
 # for SEEK_SET etc, Fcntl doesn't provide these in 5.005_03
 use IO::Seekable;
 
@@ -273,6 +273,20 @@ SKIP:
   is($io->getc, ord "P", "check we got back to the start");
 }
 
+{ # test closecb result is propagated
+  my $success_cb = sub { 1 };
+  my $failure_cb = sub { 0 };
+
+  {
+    my $io = Imager::io_new_cb(undef, $success_cb, undef, $success_cb);
+    is($io->close(), 0, "test successful close");
+  }
+  {
+    my $io = Imager::io_new_cb(undef, $success_cb, undef, $failure_cb);
+    is($io->close(), -1, "test failed close");
+  }
+}
+
 Imager->close_log;
 
 unless ($ENV{IMAGER_KEEP_FILES}) {