[Libguestfs] [libnbd PATCH] docs: Clarify how callbacks should handle errors

[Eric Blake]

On Thu, Feb 03, 2022 at 11:35:49AM +0100, Laszlo Ersek wrote:
> On 02/02/22 17:58, Eric Blake wrote:
> > Recent patches have demonstrated confusion on which order callbacks
> > are reached, when it is safe or dangerous to ignore *error, and what a
> > completion callback should do when auto-retirement is in use.  Add
> > wording to make it more obvious that:
> > 
> > - mid-command callbacks are reached before the completion callback,
> 
> I think the documentation changes kind of imply this order, but I don't
> see it stated explicitly, succinctly. Can you add just one sentence
> somewhere that simply states this?

Will do; v2 coming up, as it turned into more than just one sentence,
when I also considered Nir's comments.

> 
> >   and returning -1 does not prevent future callbacks
> > - ignoring *error in a mid-command callback is safe
> > - completion callbacks are reached unconditionally, and must NOT ignore
> >   *error
> > - if auto-retirement is in use, completion callbacks should always use
> >   it rather than trying to return -1 on error
> 
> I don't understand this. According to "docs/libnbd.pod" (and my current
> understanding), auto-retirement is *only* the case when the completion
> callback returns 1. Substituting that into the above paragraph, the
> latter becomes:
> 
> - if the completion callback returns 1, rather than the "AIO submit"
>   client logic calling nbd_aio_command_completed() manually, then the
>   completion callback should always return 1, rather than trying to
>   return -1 on error.
> 
> It seems to suggest that, with auto-retirement, there is no way to
> report an error. If that's actually *intended* (IOW, only use
> auto-retirement when you don't care about reporting errors), then I
> think the docs should be more explicit about that.
> 
> ... Hmm, OK, I think the actual documentation change is mostly
> understandable below, I'm only confused by the commit message. Basically
> the idea is, "if you *decide* to use auto-retirement, then use it
> consistently, and return 1 on *all* exit paths, even those handling
> errors -- so that you don't have to handle errors in the original AIO
> submitting code (with manual completion) in *some* cases only".

Here, I think better word-smithing in my commit message will solve the issue.

> 
> How about something like:
> 
> s/auto-retirement is in use/you choose to retire asynchronous commands
> automatically, rather than manually/

Yes, that sounds better.

> 
> > - the contents of buf after nbd_pread and friends is unspecified on
> >   error
> 
> Undefined perhaps? Unspecified means that the buffer can contain
> anything, and it's valid for the caller to access the buffer. I'd think
> it's invalid to read such buffers though.
> 
> Example: we have a local ("block scope, automatic storage duration")
> uint8_t array with 512 elements, and no initializer. In the containing
> function, we call nbd_aio_pread(), targeting the buffer, and then we
> wait for completion too. (No automatic retirement.) If
> nbd_aio_command_completed() returns -1, then the array may still have
> indeterminate value, and reading that is undefined behavior in C.

Or put another way, when using valgrind, unspecified contents do not
trigger a valgrind notice, but undefined contents can.  Yes, I'll use
the more accurate word.

> 
> Otherwise, the hunks below are in sync with the commit message, so I
> have no additional comments. If you agree with some of my suggestions
> for the commit message, then the patch body should reflect those too.
> 
> Thank you very much for having added this patch to your todo list :)

No problem - ambiguous documentation still counts as a bug worth
fixing :)

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