Re: [Libguestfs] [libnbd PATCH] api: Add way to avoid structured replies
On Wed, Sep 04, 2019 at 01:28:08PM -0500, Eric Blake wrote:
We want to default to requesting structured replies, whether or not
that request will be honored (it's essential for efficient sparse file
reads and the DF flag for structured pread, as well as for meta
context support even if we do not request a default meta context).
However, for integration testing, it can be nice to easily request a
client that does not request structured replies.
We can test this by reusing our eflags test. Note that nbdkit does
not provide a 'can_df' callback in the sh script (so our key=value
override is silently ignored), rather, we control whether nbdkit
advertises df based on whether we request structured replies.
---
I'm open to renaming the API to the shorter 'nbd_set_request_sr' if
the existing name choice seems too long.
There's not really a limit on the length of API names, and in this
case the longer name explains what the option does more clearly.
Anyway this looks fine to me.
ACK
I have one comment unrelated to the patch:
+ "set_request_structured_replies", {
+ default_call with
+ args = [Bool "request"]; ret = RErr;
+ permitted_states = [ Created ];
+ first_version = (1, 2);
I just know that we're going to end up adding new APIs and forgetting
to set the first_version field. There are various things we could do
to prevent this:
(1) In ‘default_call’ set first_version = (0, 0). Update all existing
calls with first_version = (1, 0). Then add a check that
first_version <> (0, 0). There's still a danger of copy and paste
from an existing API, but we should be able to catch that in review.
(2) Store first_version in a separate table. Add checks to ensure the
new table exhaustively covers all APIs. It should be obvious when
submitting a new API that the first_version table must be updated and
what to add here:
let first_version = [
"set_debug", (1, 0);
...
"set_request_structured_replies", (1, 2);
"get_request_structured_replies", (1, 2);
]
Not sure which is better.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html