Libguestfs - Jul 2019 - Re: [libnbd] notify API changes (was: Re: [libnbd PATCH 5/6] api: Add new nbd_aio_FOO_notify functions)

On Sat, Jun 29, 2019 at 08:28:28AM -0500, Eric Blake
wrote:
> As mentioned in the previous patch, there are situations where an aio
> client wants instant notification when a given command is complete,
> rather than having to maintain a separate data structure to track all
> in-flight commands and then iterate over that structure to learn which
> commands are complete.  It's also desirable when writing a server
> validation program (such as for checking structured reads for
> compliance) to be able to clean up the associated opaque data and have
> a final chance to change the overall command status.
> 
> Introduce new nbd_aio_FOO_notify functions for each command. Rewire
> the existing nbd_aio_FOO to forward to the new command.  (Perhaps the
> generator could reduce some of the boilerplate duplication, if a later
> patch wants to refactor this).
I'm writing some code now using these new nbd_aio_<CMD>_notify
functions, and I temporarily confused myself because these are similar
to nbd_notify_read and nbd_notify_write (the functions used to signal
to the state machine that the socket is ready for reading/writing).

I wonder if we should rename something here.  My suggestions are
either of the following or both:

(I) Rename nbd_notify_read / nbd_notify_write to nbd_ready_to_read /
nbd_ready_to_write.

(II) Rename nbd_aio_<CMD>_notify to nbd_aio_<CMD>_callback.

What do you think?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 7/15/19 6:06 AM, Richard W.M. Jones wrote:
> On Sat, Jun 29, 2019 at 08:28:28AM -0500, Eric Blake wrote:
>> As mentioned in the previous patch, there are situations where an aio
>> client wants instant notification when a given command is complete,
>> rather than having to maintain a separate data structure to track all
>> in-flight commands and then iterate over that structure to learn which
>> commands are complete.  It's also desirable when writing a server
>> validation program (such as for checking structured reads for
>> compliance) to be able to clean up the associated opaque data and have
>> a final chance to change the overall command status.
>>
>> Introduce new nbd_aio_FOO_notify functions for each command. Rewire
>> the existing nbd_aio_FOO to forward to the new command.  (Perhaps the
>> generator could reduce some of the boilerplate duplication, if a later
>> patch wants to refactor this).
> 
> I'm writing some code now using these new nbd_aio_<CMD>_notify
> functions, and I temporarily confused myself because these are similar
> to nbd_notify_read and nbd_notify_write (the functions used to signal
> to the state machine that the socket is ready for reading/writing).
> 
> I wonder if we should rename something here.  My suggestions are
> either of the following or both:
> 
> (I) Rename nbd_notify_read / nbd_notify_write to nbd_ready_to_read /
> nbd_ready_to_write.
I'm still wondering if we want to add an nbd_notify_error for the I/O
thread to inform libnbd about a POLLERR situation (as otherwise the
libnbd state machine may not notice the error until much later).  I
don't think nbd_read_to_error() sounds good, so keeping the name
nbd_notify_FOO for notifying the state machine about a particular change
in the fd seems best.
> 
> (II) Rename nbd_aio_<CMD>_notify to nbd_aio_<CMD>_callback.
which leaves this as our only sane option to rename. The bikeshedding
rename sounds fine to me; it's a fairly simple change; it does bump
API/ABI and will require another release, but we're still at the point
where we can do it.

I've also noticed that we have 'struct nbd_handle' and also refer to
all
of our nbd_aio_FOO() results as command handles; that might get
confusing. My though there is to rename the latter to 'cookie' or
'id'
or any other term you can think of which would fit (which is more of a
doc change, and not an actual API change).
> 
> What do you think?
Do you want me to go ahead and rename nbd_aio_FOO_notify to
nbd_aio_FOO_callback?

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

On Mon, Jul 15, 2019 at 08:54:41AM -0500, Eric Blake
wrote:
> On 7/15/19 6:06 AM, Richard W.M. Jones wrote:
> > (II) Rename nbd_aio_<CMD>_notify to
nbd_aio_<CMD>_callback.
> 
> which leaves this as our only sane option to rename. The bikeshedding
> rename sounds fine to me; it's a fairly simple change; it does bump
> API/ABI and will require another release, but we're still at the point
> where we can do it.
> 
> I've also noticed that we have 'struct nbd_handle' and also
refer to all
> of our nbd_aio_FOO() results as command handles; that might get
> confusing. My though there is to rename the latter to 'cookie' or
'id'
> or any other term you can think of which would fit (which is more of a
> doc change, and not an actual API change).
"Cookie" sounds fine to me.
> > What do you think?
> 
> Do you want me to go ahead and rename nbd_aio_FOO_notify to
> nbd_aio_FOO_callback?
Sure if you don't mind!  (Or I can do it, as you wish)

Thanks,

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