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

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.