From: Tony Cook <tony@develop-help.com>
Date: Wed, 25 Apr 2012 02:18:10 +0000 (+1000)
Subject: [rt #76782] improve documentation and error reporting for missing I/O callbacks
X-Git-Tag: v0.90~11
X-Git-Url: http://git.imager.perl.org/imager.git/commitdiff_plain/9d5ff8a67a886bd03610c377b0409b0ff2ed02b9

[rt #76782] improve documentation and error reporting for missing I/O callbacks
---

diff --git a/Changes b/Changes
index 24d2d2f7..b05a9d59 100644
--- a/Changes
+++ b/Changes
@@ -28,6 +28,18 @@ Bug fixes:
 
  - odd length RLE4 runs in BMP images were decoded incorrectly.
 
+Other changes:
+
+ - when reading or writing images via callbacks, the default callbacks
+   now push an error message indicating that a required callback was
+   called but not supplied.
+   https://rt.cpan.org/Ticket/Display.html?id=76782
+
+ - clarify which callbacks are required for reading and writing TIFF
+   images.
+
+ - improve logging for creation of callback I/O layers.
+
 Imager 0.89 - 18 Mar 2012
 ===========
 
diff --git a/Imager.xs b/Imager.xs
index c869f552..171a7385 100644
--- a/Imager.xs
+++ b/Imager.xs
@@ -181,8 +181,11 @@ call_reader(struct cbdata *cbd, void *buf, size_t size,
   SV *data;
   dSP;
 
-  if (!SvOK(cbd->readcb))
+  if (!SvOK(cbd->readcb)) {
+    mm_log((1, "read callback called but no readcb supplied\n"));
+    i_push_error(0, "read callback called but no readcb supplied");
     return -1;
+  }
 
   ENTER;
   SAVETMPS;
@@ -230,8 +233,11 @@ io_seeker(void *p, off_t offset, int whence) {
   off_t result;
   dSP;
 
-  if (!SvOK(cbd->seekcb))
+  if (!SvOK(cbd->seekcb)) {
+    mm_log((1, "seek callback called but no seekcb supplied\n"));
+    i_push_error(0, "seek callback called but no seekcb supplied");
     return -1;
+  }
 
   ENTER;
   SAVETMPS;
@@ -266,8 +272,11 @@ io_writer(void *p, void const *data, size_t size) {
   dSP;
   bool success;
 
-  if (!SvOK(cbd->writecb))
+  if (!SvOK(cbd->writecb)) {
+    mm_log((1, "write callback called but no writecb supplied\n"));
+    i_push_error(0, "write callback called but no writecb supplied");
     return -1;
+  }
 
   ENTER;
   SAVETMPS;
@@ -351,6 +360,27 @@ do_io_new_buffer(pTHX_ SV *data_sv) {
   return io_new_buffer(data, length, my_SvREFCNT_dec, data_sv);
 }
 
+static const char *
+describe_sv(SV *sv) {
+  if (SvOK(sv)) {
+    if (SvROK(sv)) {
+      svtype type = SvTYPE(SvRV(sv));
+      switch (type) {
+      case SVt_PVCV: return "CV";
+      case SVt_PVGV: return "GV";
+      case SVt_PVLV: return "LV";
+      default: return "some reference";
+      }
+    }
+    else {
+      return "non-reference scalar";
+    }
+  }
+  else {
+    return "undef";
+  }
+}
+
 static i_io_glue_t *
 do_io_new_cb(pTHX_ SV *writecb, SV *readcb, SV *seekcb, SV *closecb) {
   struct cbdata *cbd;
@@ -361,6 +391,8 @@ do_io_new_cb(pTHX_ SV *writecb, SV *readcb, SV *seekcb, SV *closecb) {
   cbd->seekcb = newSVsv(seekcb);
   cbd->closecb = newSVsv(closecb);
 
+  mm_log((1, "do_io_new_cb(writecb %p (%s), readcb %p (%s), seekcb %p (%s), closecb %p (%s))\n", writecb, describe_sv(writecb), readcb, describe_sv(readcb), seekcb, describe_sv(seekcb), closecb, describe_sv(closecb)));
+
   return io_new_cb(cbd, io_reader, io_writer, io_seeker, io_closer, 
 		   io_destroyer);
 }
diff --git a/lib/Imager/Files.pod b/lib/Imager/Files.pod
index 916fcf65..9e2f855c 100644
--- a/lib/Imager/Files.pod
+++ b/lib/Imager/Files.pod
@@ -273,12 +273,31 @@ When reading from a file you can use either C<callback> or C<readcb>
 to supply the read callback, and when writing C<callback> or
 C<writecb> to supply the write callback.
 
-Some file formats, currently only C<TIFF>, also require a C<seekcb>
-parameter to change position in the file.  If no C<seekcb> parameter
-is provided a default will be provided that fails.
+Whether reading or writing a C<TIFF> image, C<seekcb> and C<readcb>
+are required.
+
+If a file handler attempts to use C<readcb>, C<writecb> or C<seekcb>
+and you haven't supplied one, the call will fail, failing the image
+read or write, returning an error message indicating that the callback
+is missing:
+
+  # attempting to read a TIFF image without a seekcb
+  open my $fh, "<", $filename or die;
+  my $rcb = sub {
+    my $val;
+    read($fh, $val, $_[0]) or return "";
+    return $val;
+  };
+  my $im = Imager->new(callback => $rcb)
+    or die Imager->errstr
+  # dies with (wrapped here):
+  # Error opening file: (Iolayer): Failed to read directory at offset 0:
+  # (Iolayer): Seek error accessing TIFF directory: seek callback called
+  # but no seekcb supplied
 
 You can also provide a C<closecb> parameter called when writing the
-file is complete. 
+file is complete.  If no C<closecb> is supplied the default will
+succeed silently.
 
   # contrived
   my $data;
@@ -1068,6 +1087,15 @@ some page other than the first.  The page is 0 based:
 If you read an image with multiple alpha channels, then only the first
 alpha channel will be read.
 
+When reading a C<TIFF> image with callbacks, the C<seekcb> callback
+parameter is also required.
+
+When writing a C<TIFF> image with callbacks, the C<seekcb> and
+C<readcb> parameters are also required.
+
+C<TIFF> is a random access file format, it cannot be read from or
+written to unseekable streams such as pipes or sockets.
+
 =head2 BMP (Windows Bitmap)
 
 Imager can write 24-bit RGB, and 8, 4 and 1-bit per pixel paletted
diff --git a/t/t07iolayer.t b/t/t07iolayer.t
index bead870b..44748973 100644
--- a/t/t07iolayer.t
+++ b/t/t07iolayer.t
@@ -1,6 +1,6 @@
 #!perl -w
 use strict;
-use Test::More tests => 246;
+use Test::More tests => 252;
 # for SEEK_SET etc, Fcntl doesn't provide these in 5.005_03
 use IO::Seekable;
 
@@ -808,6 +808,32 @@ SKIP:
   }
 }
 
+{ # based on discussion on IRC, user was attempting to write a TIFF
+  # image file with only a write callback, but TIFF requires seek and
+  # read callbacks when writing.
+  # https://rt.cpan.org/Ticket/Display.html?id=76782
+  my $cb = Imager::io_new_cb(undef, undef, undef, undef);
+  {
+    Imager::i_clear_error();
+    my $data;
+    is($cb->read($data, 10), undef, "default read callback should fail");
+    is(Imager->_error_as_msg(), "read callback called but no readcb supplied",
+       "check error message");
+  }
+  {
+    Imager::i_clear_error();
+    is($cb->raw_write("abc"), -1, "default write callback should fail");
+    is(Imager->_error_as_msg(), "write callback called but no writecb supplied",
+       "check error message");
+  }
+  {
+    Imager::i_clear_error();
+    is($cb->seek(0, 0), -1, "default seek callback should fail");
+    is(Imager->_error_as_msg(), "seek callback called but no seekcb supplied",
+       "check error message");
+  }
+}
+
 Imager->close_log;
 
 unless ($ENV{IMAGER_KEEP_FILES}) {
diff --git a/t/x20spell.t b/t/x20spell.t
index 4b97643b..74e4e46b 100644
--- a/t/x20spell.t
+++ b/t/x20spell.t
@@ -52,6 +52,7 @@ preloads
 renderer
 tuple
 unary
+unseekable
 varargs
 /;