Libguestfs - Feb 2017 - Re: [PATCH 1/2] Define .errno_is_preserved constant instead of a .errno_is_reliable callback.

See patch 1 for rationale.

The callback doesn't make much sense: Could the value change
per-connection?  Unlikely.  This is a property of the plugin as a
whole.

I changed the name to "errno_is_preserved", because it's not about
the
reliability of errno, but about whether errno is preserved across
calls.
---
 docs/nbdkit-plugin.pod                  | 37 +++++++++------------------------
 include/nbdkit-plugin.h                 |  2 +-
 plugins/ocaml/nbdkit-ocaml-plugin.pod   | 11 ----------
 plugins/ocaml/ocaml.c                   | 11 ----------
 plugins/perl/nbdkit-perl-plugin.pod     |  6 ------
 plugins/perl/perl.c                     | 12 -----------
 plugins/python/nbdkit-python-plugin.pod |  6 ------
 plugins/python/python.c                 | 12 -----------
 plugins/ruby/nbdkit-ruby-plugin.pod     |  6 ------
 plugins/ruby/ruby.c                     | 12 -----------
 src/connections.c                       |  7 ++-----
 src/internal.h                          |  3 +--
 src/plugins.c                           | 26 +++++++++--------------
 13 files changed, 24 insertions(+), 127 deletions(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 3611244..4b364f3 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -147,10 +147,16 @@ C<nbdkit_error> to report an error message;
additionally, if the
 callback is involved in serving data, the plugin should call
 C<nbdkit_set_error> to influence the error code that will be sent to
 the client.  These two functions can be called in either order.  Then,
-the callback should return the appropriate error indication, eg. NULL
-or -1.  If the call to C<nbdkit_set_error> is omitted while serving
-data, then the C<.errno_is_reliable> callback controls whether to fall
-back to the value of C<errno> or a hard-coded C<EIO>.
+the callback should return the appropriate error indication,
+eg. C<NULL> or C<-1>.
+
+If the call to C<nbdkit_set_error> is omitted while serving data, then
+the global variable C<errno> may be used.  For plugins which have
+C<.errno_is_preserved == 1> the core code will use C<errno>.  In
+plugins written in non-C languages, we usually cannot trust that
+C<errno> will not be overwritten when returning from that language to
+C.  In that case, either the plugin must call C<nbdkit_set_error> or
+hard-coded C<EIO> is used.
 
 C<nbdkit_error> has the following prototype and works like
 L<printf(3)>:
@@ -386,29 +392,6 @@ error message and return C<-1>.
 This callback is not required.  If omitted, then we return true iff a
 C<.trim> callback has been defined.
 
-=head2 C<.errno_is_reliable>
-
- int errno_is_reliable (void *handle);
-
-This is called during the option negotiation phase to find out what
-errors to reflect to the client.  An error explicitly reported to
-C<nbdkit_set_error> takes precedence, but plugins written in C can
-often get by with implicitly using the value stored in C<errno>, when
-this callback returns true. On the other hand, a plugin written in any
-other language generally cannot control the value of C<errno> after
-the glue code of the language binding wrappers; returning false forces
-the use of C<EIO> as the fallback error rather than using an
-unreliable value in C<errno>.  This callback is not needed in the
-bindings of other languages.  The results of this callback do not
-matter if all data serving callbacks correctly call
-C<nbdkit_set_error>.
-
-If there is an error, C<.errno_is_reliable> should call
C<nbdkit_error>
-with an error message and return C<-1>.
-
-This callback is not required.  If omitted, then we return true for
-a plugin written in C, and false for plugins in other languages.
-
 =head2 C<.pread>
 
  int pread (void *handle, void *buf, uint32_t count, uint64_t offset);
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index 568aaf5..95cba8d 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -92,7 +92,7 @@ struct nbdkit_plugin {
   int (*trim) (void *handle, uint32_t count, uint64_t offset);
   int (*zero) (void *handle, uint32_t count, uint64_t offset, int may_trim);
 
-  int (*errno_is_reliable) (void *handle);
+  int errno_is_preserved;
 
   /* int (*set_exportname) (void *handle, const char *exportname); */
 };
diff --git a/plugins/ocaml/nbdkit-ocaml-plugin.pod
b/plugins/ocaml/nbdkit-ocaml-plugin.pod
index 250c1da..8f67d9b 100644
--- a/plugins/ocaml/nbdkit-ocaml-plugin.pod
+++ b/plugins/ocaml/nbdkit-ocaml-plugin.pod
@@ -89,17 +89,6 @@ handle struct in your plugin:
    printf "handle ID = %d\n" handle.h_id;
    (* ... *)
 
-=head2 MISSING CALLBACKS
-
-=over 4
-
-=item Missing: C<errno_is_reliable>
-
-This is not needed because the process of gluing OCaml code into C cannot
-reliably use C<errno>.
-
-=back
-
 =head2 THREADS
 
 The first parameter of C<NBDKit.register_plugin> is the thread model,
diff --git a/plugins/ocaml/ocaml.c b/plugins/ocaml/ocaml.c
index 5d7aeeb..b38789a 100644
--- a/plugins/ocaml/ocaml.c
+++ b/plugins/ocaml/ocaml.c
@@ -499,16 +499,6 @@ SET(pwrite)
 SET(flush)
 SET(trim)
 
-/* We can't guarantee that errno is stable across language binding
- * glue code, so this callback is implemented in C only, and not
- * exposed in OCaml.
- */
-static int
-plugin_ocaml_errno_is_reliable (void *handle)
-{
-  return 0;
-}
-
 /* We can't directly use NBDKIT_REGISTER_PLUGIN(). */
 struct nbdkit_plugin *
 plugin_init (void)
@@ -517,6 +507,5 @@ plugin_init (void)
     fprintf (stderr, "error: OCaml code did not call
NBDKit.register_plugin\n");
     exit (EXIT_FAILURE);
   }
-  plugin.errno_is_reliable = plugin_ocaml_errno_is_reliable;
   return &plugin;
 }
diff --git a/plugins/perl/nbdkit-perl-plugin.pod
b/plugins/perl/nbdkit-perl-plugin.pod
index 920bdef..2ecdcf1 100644
--- a/plugins/perl/nbdkit-perl-plugin.pod
+++ b/plugins/perl/nbdkit-perl-plugin.pod
@@ -327,12 +327,6 @@ C<Nbdkit::set_error(POSIX::EOPNOTSUPP)>.
 These are not needed because you can just use regular Perl C<BEGIN>
 and C<END> constructs.
 
-=item Missing: C<errno_is_reliable>
-
-This is not needed because the process of gluing Perl code into C cannot
-reliably use C<errno>.  Instead, call C<Nbdkit::set_error> when
reporting
-a failure.
-
 =item Missing: C<name>, C<version>, C<longname>,
C<description>, C<config_help>
 
 These are not yet supported.
diff --git a/plugins/perl/perl.c b/plugins/perl/perl.c
index 317a775..d9ad842 100644
--- a/plugins/perl/perl.c
+++ b/plugins/perl/perl.c
@@ -647,16 +647,6 @@ perl_trim (void *handle, uint32_t count, uint64_t offset)
   return 0;
 }
 
-/* We can't guarantee that errno is stable across language binding
- * glue code, so this callback is implemented in C only, and not
- * exposed in Perl.
- */
-static int
-perl_errno_is_reliable (void *handle)
-{
-  return 0;
-}
-
 #define perl_config_help \
   "script=<FILENAME>     (required) The Perl plugin to run.\n"
\
   "[other arguments may be used by the plugin that you load]"
@@ -688,8 +678,6 @@ static struct nbdkit_plugin plugin = {
   .flush             = perl_flush,
   .trim              = perl_trim,
   .zero              = perl_zero,
-
-  .errno_is_reliable = perl_errno_is_reliable,
 };
 
 NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 1c57e15..90395b9 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -246,12 +246,6 @@ use C<nbdkit.set_error(errno.EOPNOTSUPP)>.
 These are not needed because you can just use ordinary Python
 constructs.
 
-=item Missing: C<errno_is_reliable>
-
-This is not needed because the process of gluing Python code into C cannot
-reliably use C<errno>.  Instead, call C<nbdkit.set_error> when
reporting
-a failure.
-
 =item Missing: C<name>, C<version>, C<longname>,
C<description>, C<config_help>
 
 These are not yet supported.
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 895a361..c4ac92d 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -612,16 +612,6 @@ py_can_trim (void *handle)
     return 0;
 }
 
-/* We can't guarantee that errno is stable across language binding
- * glue code, so this callback is implemented in C only, and not
- * exposed in Python.
- */
-static int
-py_errno_is_reliable (void *handle)
-{
-  return 0;
-}
-
 #define py_config_help \
   "script=<FILENAME>     (required) The Python plugin to
run.\n" \
   "[other arguments may be used by the plugin that you load]"
@@ -653,8 +643,6 @@ static struct nbdkit_plugin plugin = {
   .flush             = py_flush,
   .trim              = py_trim,
   .zero              = py_zero,
-
-  .errno_is_reliable = py_errno_is_reliable,
 };
 
 NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/ruby/nbdkit-ruby-plugin.pod
b/plugins/ruby/nbdkit-ruby-plugin.pod
index 6cf8e97..980e009 100644
--- a/plugins/ruby/nbdkit-ruby-plugin.pod
+++ b/plugins/ruby/nbdkit-ruby-plugin.pod
@@ -251,12 +251,6 @@ use C<Nbdkit.set_error(Errno::EOPNOTSUPP)>.
 These are not needed because you can just use ordinary Ruby
 constructs.
 
-=item Missing: C<errno_is_reliable>
-
-This is not needed because the process of gluing Ruby code into C cannot
-reliably use C<errno>.  Instead, call C<Nbdkit.set_error> when
reporting
-a failure.
-
 =item Missing: C<name>, C<version>, C<longname>,
C<description>, C<config_help>
 
 These are not yet supported.
diff --git a/plugins/ruby/ruby.c b/plugins/ruby/ruby.c
index 33d7968..023c1e8 100644
--- a/plugins/ruby/ruby.c
+++ b/plugins/ruby/ruby.c
@@ -467,16 +467,6 @@ plugin_rb_can_trim (void *handle)
   return RTEST (rv);
 }
 
-/* We can't guarantee that errno is stable across language binding
- * glue code, so this callback is implemented in C only, and not
- * exposed in Ruby.
- */
-static int
-plugin_rb_errno_is_reliable (void *handle)
-{
-  return 0;
-}
-
 #define plugin_rb_config_help \
   "script=<FILENAME>     (required) The Ruby plugin to run.\n"
\
   "[other arguments may be used by the plugin that you load]"
@@ -511,8 +501,6 @@ static struct nbdkit_plugin plugin = {
   .flush             = plugin_rb_flush,
   .trim              = plugin_rb_trim,
   .zero              = plugin_rb_zero,
-
-  .errno_is_reliable = plugin_rb_errno_is_reliable,
 };
 
 NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/src/connections.c b/src/connections.c
index cf06ac5..a0d689a 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -498,10 +498,7 @@ negotiate_handshake (struct connection *conn)
   int r;
 
   plugin_lock_request (conn);
-  conn->errno_is_reliable = plugin_errno_is_reliable (conn);
-  if (conn->errno_is_reliable < 0)
-    r = -1;
-  else if (!newstyle)
+  if (!newstyle)
     r = _negotiate_handshake_oldstyle (conn);
   else
     r = _negotiate_handshake_newstyle (conn);
@@ -612,7 +609,7 @@ get_error (struct connection *conn)
 {
   int ret = tls_get_error ();
 
-  if (!ret && conn->errno_is_reliable)
+  if (!ret && plugin_errno_is_preserved ())
     ret = errno;
   return ret ? ret : EIO;
 }
diff --git a/src/internal.h b/src/internal.h
index aa7dbd3..e73edf1 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -120,7 +120,6 @@ struct connection {
   int can_flush;
   int is_rotational;
   int can_trim;
-  int errno_is_reliable;
 };
 
 extern int handle_single_connection (int sockin, int sockout);
@@ -141,10 +140,10 @@ extern void plugin_lock_connection (void);
 extern void plugin_unlock_connection (void);
 extern void plugin_lock_request (struct connection *conn);
 extern void plugin_unlock_request (struct connection *conn);
+extern int plugin_errno_is_preserved (void);
 extern int plugin_open (struct connection *conn, int readonly);
 extern void plugin_close (struct connection *conn);
 extern int64_t plugin_get_size (struct connection *conn);
-extern int plugin_errno_is_reliable (struct connection *conn);
 extern int plugin_can_write (struct connection *conn);
 extern int plugin_can_flush (struct connection *conn);
 extern int plugin_is_rotational (struct connection *conn);
diff --git a/src/plugins.c b/src/plugins.c
index 837a54f..eeed8a9 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -236,6 +236,7 @@ plugin_dump_fields (void)
     break;
   }
   printf ("\n");
+  printf ("errno_is_preserved=%d\n", plugin.errno_is_preserved);
 
 #define HAS(field) if (plugin.field) printf ("has_%s=1\n", #field)
   HAS (longname);
@@ -350,6 +351,14 @@ plugin_unlock_request (struct connection *conn)
 }
 
 int
+plugin_errno_is_preserved (void)
+{
+  assert (dl);
+
+  return plugin.errno_is_preserved;
+}
+
+int
 plugin_open (struct connection *conn, int readonly)
 {
   void *handle;
@@ -395,21 +404,6 @@ plugin_get_size (struct connection *conn)
 }
 
 int
-plugin_errno_is_reliable (struct connection *conn)
-{
-  assert (dl);
-  assert (conn->handle);
-
-  debug ("errno_is_reliable");
-
-  if (plugin.errno_is_reliable)
-    return plugin.errno_is_reliable (conn->handle);
-
-  /* Default to 1, for backwards compatibility (correct for C plugins) */
-  return 1;
-}
-
-int
 plugin_can_write (struct connection *conn)
 {
   assert (dl);
@@ -548,7 +542,7 @@ plugin_zero (struct connection *conn,
     result = plugin.zero (conn->handle, count, offset, may_trim);
     if (result == -1) {
       err = tls_get_error ();
-      if (!err && conn->errno_is_reliable)
+      if (!err && plugin_errno_is_preserved ())
         err = errno;
     }
     if (result == 0 || err != EOPNOTSUPP)
-- 
2.10.2

---
 plugins/example2/example2.c   | 4 ++++
 plugins/example3/example3.c   | 4 ++++
 plugins/file/file.c           | 1 +
 plugins/streaming/streaming.c | 1 +
 4 files changed, 10 insertions(+)

diff --git a/plugins/example2/example2.c b/plugins/example2/example2.c
index a9a0e37..0fd0421 100644
--- a/plugins/example2/example2.c
+++ b/plugins/example2/example2.c
@@ -193,6 +193,10 @@ static struct nbdkit_plugin plugin = {
   .close             = example2_close,
   .get_size          = example2_get_size,
   .pread             = example2_pread,
+  /* In this plugin, errno is preserved properly along error return
+   * paths from failed system calls.
+   */
+  .errno_is_preserved = 1,
 };
 
 NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/example3/example3.c b/plugins/example3/example3.c
index 0c215ab..8ac39ae 100644
--- a/plugins/example3/example3.c
+++ b/plugins/example3/example3.c
@@ -214,6 +214,10 @@ static struct nbdkit_plugin plugin = {
   .pread             = example3_pread,
   .pwrite            = example3_pwrite,
   .flush             = example3_flush,
+  /* In this plugin, errno is preserved properly along error return
+   * paths from failed system calls.
+   */
+  .errno_is_preserved = 1,
 };
 
 NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/file/file.c b/plugins/file/file.c
index 655fba7..bfa20fb 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -336,6 +336,7 @@ static struct nbdkit_plugin plugin = {
   .pwrite            = file_pwrite,
   .zero              = file_zero,
   .flush             = file_flush,
+  .errno_is_preserved = 1,
 };
 
 NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/streaming/streaming.c b/plugins/streaming/streaming.c
index fb2d3ae..c2deeb6 100644
--- a/plugins/streaming/streaming.c
+++ b/plugins/streaming/streaming.c
@@ -256,6 +256,7 @@ static struct nbdkit_plugin plugin = {
   .get_size          = streaming_get_size,
   .pwrite            = streaming_pwrite,
   .pread             = streaming_pread,
+  .errno_is_preserved = 1,
 };
 
 NBDKIT_REGISTER_PLUGIN(plugin)
-- 
2.10.2

On 02/06/2017 08:57 AM, Richard W.M. Jones wrote:
> The callback doesn't make much sense: Could the value change
> per-connection?  Unlikely.  This is a property of the plugin as a
> whole.
> 
> I changed the name to "errno_is_preserved", because it's not
about the
> reliability of errno, but about whether errno is preserved across
> calls.
Makes it possible for a regression in C plugins (previously, such
plugins implicitly behaved as if .errno_is_preserved was 1, now such
plugins default to .errno_is_preserved == 0 unless explicitly set and
recompiled against newer headers).  Thus, such plugins will now fail
with EIO where they used to fail with reasonable errno values.

But I guess we can live with that.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org