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

[Eric Blake]

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.

> 
> 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.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170126/8eeda0f9/attachment.sig>