[Libguestfs] [PATCH libnbd v3 1/2] lib: Implement closure lifetimes.

[Eric Blake]

On 7/25/19 12:01 PM, Richard W.M. Jones wrote:
> On Thu, Jul 25, 2019 at 10:43:05AM -0500, Eric Blake wrote:
>> On 7/25/19 8:07 AM, Richard W.M. Jones wrote:
>>> @@ -499,7 +499,7 @@
>>>        /* Call the caller's extent function. */
>>>        int error = cmd->error;
>>>  
>>> -      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
>>> +      if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
>>>                               meta_context->name, cmd->offset,
>>>                               &h->bs_entries[1], (length-4) / 4, &error) == -1)
>>>          if (cmd->error == 0)
>>
>> Hmm - no change to the FINISH state, which means you are relying on
>> command retirement to free chunk/extent instead.  As long as that
>> happens, we should be okay, though.
> 
> I didn't understand this one.  Is it wrong to add the VALID
> flag here?

No, the VALID flag here is correct; my comment (with insufficient
context) was tied to the fact that this is the last hunk of changes to
this file, which means we did not change the FINISH state to call
read(FREE) or extent(FREE) (and instead defer the freeing to when we
retire the command).  So nothing to change in what you have here.

In fact, thinking a bit more - my followup patch to call
read(VALID|FREE) in more places is worthwhile (because it reduces the
number of callbacks compared to read(VALID);read(FREE)); but teaching
FINISH to call read(FREE) does not optimize how many callbacks are made,
only the point in time in which they are made (freeing the read callback
as soon as we know the command is done, and before the completion
callback runs), nor does it remove the requirement to still have code
for read(FREE) in the retirement code (for when the command is
stranded).  An argument for calling read(FREE) in FINISH is to avoid the
surprise mentioned in the other email where callback(FREE, data) &&
read(FREE, data) (because the user passed the same data to both
callbacks) results in the read callback seeing a stale pointer, but then
again, it's the caller's burden to not dereference the pointer if they
share the data between callbacks, and not necessarily a case where we
MUST call read(FREE) as soon as possible.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190725/5010a4da/attachment.sig>