Libguestfs - Sep 2022 - [libnbd PATCH v2 04/12] api: Add nbd_set_request_meta_context()

On Thu, Sep 01, 2022 at 03:38:34PM -0500, Eric Blake
wrote:
> On Thu, Sep 01, 2022 at 05:29:21PM +0100, Richard W.M. Jones wrote:
> > On Wed, Aug 31, 2022 at 09:39:20AM -0500, Eric Blake wrote:
> > > Add a new control knob nbd_set_request_meta_context(), modeled
after
> > > the existing nbd_set_request_structured_replies(), to make it
possible
> > > to skip the NBD_OPT_SET_META_CONTEXT half of the two-command
sequence
> > > currently performed in nbd_opt_go() and nbd_opt_info().  Also add
a
> > > counterpart nbd_get_request_meta_context() for symmetry.
> > > 
> > 
> > This really needs an example.
> 
> Will do.  Sounds like I have enough incremental things to improve that
> it will be worth a v3 series, although I'll continue to let more
> reviews come on in the rest of the series before I post that.
I did look at all the patches in the series and didn't have any other
comments.

TBH I don't mind if you want to post another round or just push
something.  We've got plenty of time to test and fix things up before
the next release.
> I'm wondering whether it is easier to modify the existing
> examples/list-exports.c, or to write a second example.  Over the
> course of this series, as more and more APIs are added, the example
> can cover more steps.  By the end of the series, it would look
> something like:
>
> // error checking omitted here for brevity, but I'll probably add it in
the example
> nbd = nbd_create ();
> nbd_set_tls (nbd, LIBNBD_TLS_DISABLE); // we'll manually enable tls
later
> nbd_set_request_structured_reply (nbd, false); // we'll manually
request SR later
> nbd_set_request_meta_context (nbd, false); // we'll do this ourselves
> nbd_set_opt_mode (nbd, true);
> nbd_connect_...();
> if (!nbd_aio_is_negotiating (nbd))
>   fatal ("server does not support listing");
> // Determine which exports (if any) are exposed without TLS
> printf ("unencrypted exports:\n");
> nbd_opt_list (nbd, callback_exports);
> // Now enable TLS, and try again
> if (nbd_opt_starttls (nbd) != 0)
>   fatal ("server does not support TLS");
> printf ("encrypted exports:\n");
> nbd_opt_list (nbd, callback_exports);
> printf ("which export to learn more about?");
> get user's choice...
> nbd_set_export_name (nbd, name);
> if (nbd_opt_structured_reply (nbd) == 1) {
>   printf ("metacontexts available:");
>   nbd_opt_list_meta_context (nbd, callback_contexts);
>   while (1) {
>     printf ("which context to request, or -1 to finish
connecting");
>     get user's choice
>     if (choice == -1) break;
>     nbd_add_meta_context (nbd, choice);
>   }
>   nbd_opt_set_meta_context (nbd, callback_silent);
> }
> else
>   printf ("metacontexts not available\n");
> nbd_opt_go (nbd);
I don't have a preference, but the thing to bear in mind is that
people often copy examples verbatim.

So it's best to make examples be as simple and clear as possible, and
especially not contain any "gotchas" -- eg code that if copied without
understanding will make things slow or complicated.  More examples are
fine when they demonstrate distinct things.
> > > @@ -3246,6 +3299,10 @@ let first_version > > >   
"set_request_block_size", (1, 12);
> > >    "get_request_block_size", (1, 12);
> > > 
> > > +  (* Added in 1.13.x development cycle, will be stable and
supported in 1.14. *)
> > > +  "set_request_meta_context", (1, 14);
> > > +  "get_request_meta_context", (1, 14);
> > 
> > I think this should be 1.15.x .. 1.16, since 1.14 is already out.
> 
> You're right. Fixed for the nbd_supports_vsock() patch, since that one
> will beat this series in.
Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On 09/01/22 22:46, Richard W.M. Jones wrote:
> On Thu, Sep 01, 2022 at 03:38:34PM -0500, Eric Blake wrote:
>> On Thu, Sep 01, 2022 at 05:29:21PM +0100, Richard W.M. Jones wrote:
>>> On Wed, Aug 31, 2022 at 09:39:20AM -0500, Eric Blake wrote:
>>>> Add a new control knob nbd_set_request_meta_context(), modeled
after
>>>> the existing nbd_set_request_structured_replies(), to make it
possible
>>>> to skip the NBD_OPT_SET_META_CONTEXT half of the two-command
sequence
>>>> currently performed in nbd_opt_go() and nbd_opt_info().  Also
add a
>>>> counterpart nbd_get_request_meta_context() for symmetry.
>>>>
>>>
>>> This really needs an example.
>>
>> Will do.  Sounds like I have enough incremental things to improve that
>> it will be worth a v3 series, although I'll continue to let more
>> reviews come on in the rest of the series before I post that.
> 
> I did look at all the patches in the series and didn't have any other
> comments.
> 
> TBH I don't mind if you want to post another round or just push
> something.  We've got plenty of time to test and fix things up before
> the next release.
I'd be OK with the reviewed front of the series being merged, and then a
v3 of the tail being posted (if that's not too much burden for Eric).
I'm between patches #3 and #4, so if Eric agrees, I could resume my
review with *v3* of patch#4.

I can also continue reviewing this version if that's preferred.

(The main reason I'm trying to review this series meticulously is to
learn about libnbd for the long term; it's a learning investment on my
part. It's not because I insist that the series not be merged without my
approval! :))

Laszlo
> 
>> I'm wondering whether it is easier to modify the existing
>> examples/list-exports.c, or to write a second example.  Over the
>> course of this series, as more and more APIs are added, the example
>> can cover more steps.  By the end of the series, it would look
>> something like:
>>
>> // error checking omitted here for brevity, but I'll probably add
it in the example
>> nbd = nbd_create ();
>> nbd_set_tls (nbd, LIBNBD_TLS_DISABLE); // we'll manually enable tls
later
>> nbd_set_request_structured_reply (nbd, false); // we'll manually
request SR later
>> nbd_set_request_meta_context (nbd, false); // we'll do this
ourselves
>> nbd_set_opt_mode (nbd, true);
>> nbd_connect_...();
>> if (!nbd_aio_is_negotiating (nbd))
>>   fatal ("server does not support listing");
>> // Determine which exports (if any) are exposed without TLS
>> printf ("unencrypted exports:\n");
>> nbd_opt_list (nbd, callback_exports);
>> // Now enable TLS, and try again
>> if (nbd_opt_starttls (nbd) != 0)
>>   fatal ("server does not support TLS");
>> printf ("encrypted exports:\n");
>> nbd_opt_list (nbd, callback_exports);
>> printf ("which export to learn more about?");
>> get user's choice...
>> nbd_set_export_name (nbd, name);
>> if (nbd_opt_structured_reply (nbd) == 1) {
>>   printf ("metacontexts available:");
>>   nbd_opt_list_meta_context (nbd, callback_contexts);
>>   while (1) {
>>     printf ("which context to request, or -1 to finish
connecting");
>>     get user's choice
>>     if (choice == -1) break;
>>     nbd_add_meta_context (nbd, choice);
>>   }
>>   nbd_opt_set_meta_context (nbd, callback_silent);
>> }
>> else
>>   printf ("metacontexts not available\n");
>> nbd_opt_go (nbd);
> 
> I don't have a preference, but the thing to bear in mind is that
> people often copy examples verbatim.
> 
> So it's best to make examples be as simple and clear as possible, and
> especially not contain any "gotchas" -- eg code that if copied
without
> understanding will make things slow or complicated.  More examples are
> fine when they demonstrate distinct things.
> 
>>>> @@ -3246,6 +3299,10 @@ let first_version >>>>   
"set_request_block_size", (1, 12);
>>>>    "get_request_block_size", (1, 12);
>>>>
>>>> +  (* Added in 1.13.x development cycle, will be stable and
supported in 1.14. *)
>>>> +  "set_request_meta_context", (1, 14);
>>>> +  "get_request_meta_context", (1, 14);
>>>
>>> I think this should be 1.15.x .. 1.16, since 1.14 is already out.
>>
>> You're right. Fixed for the nbd_supports_vsock() patch, since that
one
>> will beat this series in.
> 
> Rich.
>