Commit 099aa257 authored by Eric Blake's avatar Eric Blake
Browse files

generator: Actually request extended headers



This is the culmination of the previous patches' preparation work for
using extended headers when possible.  The new states in the state
machine are copied extensively from our handling of
OPT_STRUCTURED_REPLY.  The next patch will then expose a new API
nbd_opt_extended_headers() for manual control.

At the same time I posted this patch, I had patches for qemu-nbd to
support extended headers as server (nbdkit is a bit tougher).  The
next patches will add some interop tests that pass when using a new
enough qemu-nbd, showing that we have cross-project interoperability
and therefore an extension worth standardizing.

NOTE: This patch causes an observable (albeit uncommon) change in
behavior to a specific corner case involving a meta-context with
64-bit flags (so far, no such meta-context exists, but I'll use
"x-context:big" as a placeholder for such a meta-context).  An
application compiled and run with libnbd 1.16 that requests
nbd_add_meta_context(h, "x-context:big") will fail to negotiate that
context, but can still succeed at negotiating "base:allocation".
What's more, that application was compiled at a time when the
nbd_block_status_64() API did not exist, so it will necessarily be
using the older 32-bit nbd_block_status() API.  With the approach done
in this patch (that is, the same client now linking against libnbd
1.18 defaults to unconditionally requesting extended headers), the
negotiation for "x-context:big" will now succeed, but calls to
nbd_block_status() will encounter an EOVERFLOW error when the server's
64-bit flags cannot be passed back to the caller's 32-bit callback.
The caller will still see the "base:allocation" results, but the
overall API will fail.  Thus, we have taken a case that used to pass
into something that now fails.

Still, it is easy to make the following mitigating arguments: 1) most
applications _don't_ blindly request "x-context:big" while insisting
on a 32-bit API; since interpreting the context bits is
context-specific, any app that knows what those bits actually mean (or
which want to support arbitrary user input for meta contexts, like
'nbdinfo --map') will want to be compiled against libnbd 1.18 in the
first place, to take advantage of nbd_block_status_64().  2) If the
application REALLY needs to preserve 1.16 behavior, even when compiled
against 1.18 or newer, it is easy enough to add the following escape
hatch:

#if LIBNBD_HAVE_NBD_SET_REQUEST_EXTENDED_HEADERS
nbd_set_request_extended_headers(h, 0);
#endif

And since such an escape hatch is only needed in the finite (possibly
empty?) set of programs that need to preserve the older behavior, it
does not introduce scaling problems.

Because my approach is a subtle change, I also evaluated two other
potential solutions, documented here, and why I did not go with them:

Approach 2) No change to the default.  ALL applications that want to
use extended headers have to explicitly opt-in (possibly taking
advantage of LIBNBD_HAVE_... witness macros to allow successful
compilation to fallback APIs when compiling against older libnbd).
This avoids the subtle change, but comes at an open-ended cost: all
future libnbd clients that WANT to benefit from extended headers will
have to add boilerplate to their connection setup, which does not
scale well, just to benefit the few applications that depended on the
older behavior.  It becomes highly likely that someone will write a
new client but does not use the new boilerplate, and thereby
inadvertently suffers from worse performace because of the older
defaults being set in stone.

Approach 3) Resort to versioned symbols, to make the default of
whether to request extended headers (in the absence of an explicit
nbd_set_request_extended_headers() call) depend on which version of
libnbd one compiles against.  That is, teach our linker script to
produce aliased symbols, where nbd_create becomes an alias to either
__nbd_create@@1_16 (default off) for backwards compatibility to apps
compiled against older libnbd but run against the new .so, or to
__nbd_create@@1_18 (default on) with preprocessor magic in <libnbd.h>
that you get this alias by default when compiling against newer
libnbd.  This scales well for clients (old apps continue to run with
identical behavior, new apps automatically get new features unless
they request preprocessor magic to specifically favor the older
aliasing styles), but is dreadful to maintain (look at glibc for the
hoops they have to jump through for such versioned-symbol shenanigans
needed to provide transparent multi-standard support).  I don't see
libnbd as such a critical or high-user-base project that we need to
take on this extra level of work.

Signed-off-by: Eric Blake's avatarEric Blake <eblake@redhat.com>
Reviewed-by: default avatarRichard W.M. Jones <rjones@redhat.com>
Message-ID: <20230803015720.1090260-21-eblake@redhat.com>
parent b1e7cb62
Pipeline #984082127 failed with stage
in 11 minutes and 28 seconds
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment