Re: [Libguestfs] [libnbd PATCH] api: Add way to avoid structured replies

2019-09-05 Thread Eric Blake 
On 9/5/19 2:40 AM, Richard W.M. Jones wrote:

> 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.

Or maybe couple that with a declaration that maps version (1, 0) has X
signatures and version (1, 2) has Y releases, then if we detect a count
mismatch, it must be a new addition incorrectly versioned.

> 
> (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);
>   ]

This approach also seems fine (it's a bit closer to maintaining the
.syms file by hand, but with the compiler guaranteeing that we touch
.syms for everything we add).  I lean slightly towards option 2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH] api: Add way to avoid structured replies

2019-09-05 Thread Richard W.M. Jones 
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

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs