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

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.

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.

So I think if the caller didn't call nbdkit_set_errno, we should
assume no errno value is available for us to use.

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

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

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