[Libguestfs] [nbdkit PATCH v2 4/6] plugins: Add new nbdkit_set_error() utility function

[Richard W.M. Jones]

On Thu, Jan 26, 2017 at 08:21:17AM -0600, Eric Blake wrote:
> On 01/26/2017 04:13 AM, Richard W.M. Jones wrote:
> > On Wed, Jan 25, 2017 at 08:42:34PM -0600, Eric Blake wrote:
> >> +eg. NULL or -1.  If the call to C<nbdkit_set_error> is omitted, then
> >> +the value of C<errno> will be used instead.
> > [...]
> >> +/* Grab the appropriate error value.
> >> + */
> >> +static int
> >> +_get_error (void)
> >> +{
> >> +  int err = errno;
> >> +  int ret = tls_get_error ();
> >> +
> >> +  if (!ret)
> >> +    ret = err ? err : EIO;
> >> +  return ret;
> >> +}
> > 
> > I don't think we should use the implicit errno.
> 
> Pre-patch:
> 
> C plugins use implicit errno, with reasonable success - but it requires
> that the C plugins be careful to not corrupt errno during cleanup.
> Language binding plugins use implicit errno, which is almost always wrong.
> 
> > 
> > The reason is that we cannot be sure that errno is meaningful in
> > language bindings.  A lot of code could run between (eg) a Perl plugin
> > seeing a system call fail, and that plugin callback returning to
> > nbdkit code, and any of that code might touch errno.  Since some of
> > that code would be in the language interpreter, we cannot even be
> > careful about preserving errno along those paths.
> 
> Indeed - so it is a pre-existing bug.

Hmm, good point.  I forgot that we were using errno even before 19184d3e.

Well I guess the patch is OK in that case.

> > So I think if the caller didn't call nbdkit_set_errno, we should
> > assume no errno value is available for us to use.
> 
> Completely avoiding errno will make little difference to language
> binding plugins (errors will now default to EIO instead of errno if
> nbdkit_set_error() was not called, but even that error is still almost
> always wrong); but it will be a regression in quality for existing C
> plugins that aren't retrofitted to call nbdkit_set_error() everywhere.
> 
> How about this: we add a new boolean callback .errno_is_reliable(),
> which defaults to true if omitted.  C plugins that don't implement the
> new callback will continue to use implicit errno, for backwards
> compatibility and no regression; such a plugin can avoid
> nbdkit_set_error (although using it won't hurt, and will make it so that
> an accidental errno corruption during cleanup no longer matters).
> Meanwhile, all of our language bindings will implement the callback (at
> the C binding level) to return false, so that they now ignore errno
> entirely.  We don't need to expose an errno_is_reliable binding to any
> of the languages; it is a C-only callback.  Then exposing
> nbdkit_set_error through the language bindings will allow plugins to
> finally have control (rather than a completely random errno pre-patch or
> a forced EIO post-patch).
> 
> I'll respin a v3 along those lines.

It sounds like errno_is_reliable is an unknowable value.  How would we
set it correctly for Perl (for example), in a way that would work for
all past and future Perl interpreters?

It looks like v2 will be fine, no need to respin ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top