Re: [Libguestfs] [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics

Previously, we let a plugin set an error in either thread-local storage (nbdkit_set_error()) or errno, then connections.c would decode which error to use. But with filters in the mix, it is very difficult for a filter to know what error was set by the plugin (particularly since nbdkit_set_error() has no public counterpart for reading the thread-local storage). What's more, if a filter does any non-trivial processing after calling into next_ops, it is very probable that errno might be corrupted, which would affect the error returned by a plugin that relied on errno but not the error stored in thread-local storage. Better is to change the backend interface to just pass the direct error value, by moving the decoding of thread-local vs. errno into plugins.c. With the change in decoding location, the backend interface no longer needs an .errno_is_preserved() callback. For maximum convenience, this change lets a filter return an error either by passing through the underlying plugin return (a positive error) or by setting -1 and storing something in errno. However, I did have to tweak some of the existing filters to actually handle and/or return the right error; which means this IS a subtle change to the filter interface (and worse, one that cannot be detected by the compiler because the API signatures did not change). However, more ABI changes are planned with adding FUA support, so as long as it is all done as part of the same release, we are okay. Also note that only nbdkit-plugin.h declares nbdkit_set_error(); we can actually keep it this way (filters do not need to call that function). Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- docs/nbdkit-filter.pod | 83 +++++++++++++++++++++++++++++++++++++------ src/internal.h | 1 - src/connections.c | 45 ++++++----------------- src/filters.c | 81 +++++++++++++++++++++++++---------------- src/plugins.c | 66 +++++++++++++++++++--------------- filters/cache/cache.c | 49 +++++++++++++++---------- filters/cow/cow.c | 28 +++++++++------ filters/partition/partition.c | 2 +- 8 files changed, 221 insertions(+), 134 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index eb72dae..4ddf25d 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -163,10 +163,14 @@ short-circuited. The filter’s other methods like C<.prepare>, C<.get_size>, C<.pread> etc ― always called in the context of a connection ― are passed a -pointer to C<struct nbdkit_next_ops> which contains a subset of the -plugin methods that can be called during a connection. It is possible -for a filter to issue (for example) extra read calls in response to a -single C<.pwrite> call. +pointer to C<struct nbdkit_next_ops> which contains a comparable set +of accessors to plugin methods that can be called during a connection. +It is possible for a filter to issue (for example) extra read calls in +response to a single C<.pwrite> call. Note that the semantics of the +functions in C<struct nbdkit_next_ops> are slightly different from +what a plugin implements: for example, while a plugin's C<.pread> +returns -1 on error, C<next_ops->pread> returns a positive errno