Re: [libvirt] [PATCH v4 6/6] virStream*All: Report error if a callback fails
On 07/08/2017 02:52 PM, John Ferlan wrote:
On 06/22/2017 08:30 AM, Michal Privoznik wrote:
> All of these four functions (virStreamRecvAll, virStreamSendAll,
> virStreamSparseRecvAll, virStreamSparseSendAll) take one or more
> callback that handle various aspects of streams. However, if any
(same as previous review)
s/callback/callback functions/
> of them fails no error is reported therefore caller does not know
> what went wrong.
>
> At the same time, we silently presumed callbacks to set errno on
> failure. With this change we should document it explicitly as the
> error is not properly reported.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> include/libvirt/libvirt-stream.h | 17 +++++++++++++-
> src/libvirt-stream.c | 50 +++++++++++++++++++++++++++++++++-------
> 2 files changed, 58 insertions(+), 9 deletions(-)
>
I think you could move all those "errno = 0;" settings outside the "for
(;;) {" loop beginnings. Unless it's felt some called function would
change errno to something and return 0. They're not wrong where they
are, so it's your call.
Yeah I can. I just wanted to be safe. What if a function touches errno
(e.g. resets it) without returning error? Yes, such function is flawed,
but it doesn't mean we can't work with it. On the other hand, the worst
thing that can happen is we report EIO instead of real error. Also,
setting errno in each loop is overkill, right? ;-) I'll move the lines
in front of the loops.
You could also alter each of the new comments requesting the setting of
errno to something to indicate failure to set errno will cause libvirt
to default to using EIO.
I'm slightly hesitant to do this. What if we want to change the default
at some point? I'd say leave the comments as they are.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks.
Michal