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