[libvirt] [PATCH v3 0/6] Fix error reporting in streams
diff to v3: -In the last patch, instead of reporting errors from callbacks, report system error based on errno set by the callbacks. Michal Privoznik (6): virfdstream: Check for thread error more frequently fdstream: Report error from the I/O thread virStream*All: Call virStreamAbort() more frequently virStream*All: Preserve reported error virFileInData: preserve errno in cleanup path virStream*All: Report error if a callback fails daemon/stream.c | 18 ++++++--- include/libvirt/libvirt-stream.h | 17 +++++++- src/libvirt-stream.c | 83 +++++++++++++++++++++++++++++++--------- src/util/virfdstream.c | 22 ++++++++--- src/util/virfile.c | 5 ++- 5 files changed, 113 insertions(+), 32 deletions(-) -- 2.13.0
When the I/O thread quits (e.g. due to an I/O error, lseek() error, whatever), any subsequent virFDStream API should return error too. Moreover, when invoking stream event callback, we must set the VIR_STREAM_EVENT_ERROR flag so that the callback knows something bad happened. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfdstream.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 7ee58be13..cd24757e6 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -312,6 +312,9 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; } + if (fdst->threadErr) + events |= VIR_STREAM_EVENT_ERROR; + cb = fdst->cb; cbopaque = fdst->opaque; ff = fdst->ff; @@ -787,10 +790,10 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) if (fdst->thread) { char *buf; - if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { virReportSystemError(EBADF, "%s", _("cannot write to stream")); - return -1; + goto cleanup; } if (VIR_ALLOC(msg) < 0 || @@ -866,7 +869,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) virFDStreamMsgPtr msg = NULL; while (!(msg = fdst->msg)) { - if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { if (nbytes) { virReportSystemError(EBADF, "%s", _("stream is not open")); @@ -959,6 +962,9 @@ virFDStreamSendHole(virStreamPtr st, fdst->offset += length; } + if (fdst->threadErr) + goto cleanup; + if (fdst->thread) { /* Things are a bit complicated here. If FDStream is in a * read mode, then if the message at the queue head is @@ -1018,6 +1024,9 @@ virFDStreamInData(virStreamPtr st, virObjectLock(fdst); + if (fdst->threadErr) + goto cleanup; + if (fdst->thread) { virFDStreamMsgPtr msg; -- 2.13.0
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
When the I/O thread quits (e.g. due to an I/O error, lseek() error, whatever), any subsequent virFDStream API should return error too. Moreover, when invoking stream event callback, we must set the VIR_STREAM_EVENT_ERROR flag so that the callback knows something bad happened.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfdstream.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 7ee58be13..cd24757e6 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -312,6 +312,9 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; }
+ if (fdst->threadErr) + events |= VIR_STREAM_EVENT_ERROR; +
This hunk almost feels separable... That is, it's updating the events (VIR_STREAM_EVENT_ERROR) in the callback function feels different than checking for threadErr more often, but I'll leave it up to you to decide to split more or not.
cb = fdst->cb; cbopaque = fdst->opaque; ff = fdst->ff; @@ -787,10 +790,10 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) if (fdst->thread) { char *buf;
- if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { virReportSystemError(EBADF, "%s", _("cannot write to stream"));
Would this (and the subsequent similar check) possibly overwrite something from a threadErr with this error message?
- return -1; + goto cleanup;
ouch, <sigh> and this is separable too technically....
}
if (VIR_ALLOC(msg) < 0 || @@ -866,7 +869,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) virFDStreamMsgPtr msg = NULL;
while (!(msg = fdst->msg)) { - if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { if (nbytes) { virReportSystemError(EBADF, "%s", _("stream is not open")); @@ -959,6 +962,9 @@ virFDStreamSendHole(virStreamPtr st, fdst->offset += length; }
+ if (fdst->threadErr) + goto cleanup; +
So it's interesting in these paths the threadErr check is done outside the fdst->thread check which is different than the virFDStreamRead and Write API's.
if (fdst->thread) { /* Things are a bit complicated here. If FDStream is in a * read mode, then if the message at the queue head is @@ -1018,6 +1024,9 @@ virFDStreamInData(virStreamPtr st,
virObjectLock(fdst);
+ if (fdst->threadErr) + goto cleanup; +
This one in particular because threadQuit is checked in the subsequent code similar to virFDStreamRead made me ponder a bit more, but I'm not sure I could come to a conclusion... I guess I'd want to see some consistency over when threadErr is checked between the 4 functions or I'd wonder why 2 do things one way and 2 the other. If they need to be different for a specific reason, then I suppose more separation could be done or at least the commit message indicate why they're different. John
if (fdst->thread) { virFDStreamMsgPtr msg;
On 06/12/2017 11:26 PM, John Ferlan wrote:
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
When the I/O thread quits (e.g. due to an I/O error, lseek() error, whatever), any subsequent virFDStream API should return error too. Moreover, when invoking stream event callback, we must set the VIR_STREAM_EVENT_ERROR flag so that the callback knows something bad happened.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfdstream.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 7ee58be13..cd24757e6 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -312,6 +312,9 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; }
+ if (fdst->threadErr) + events |= VIR_STREAM_EVENT_ERROR; +
This hunk almost feels separable... That is, it's updating the events (VIR_STREAM_EVENT_ERROR) in the callback function feels different than checking for threadErr more often, but I'll leave it up to you to decide to split more or not.
I don't think that separation is needed. The commit says "Check for thread error more frequently". Every check comes from an action. In majority of the cases the action is reporting an error. In some cases it means setting a flag, a boolean that causes error to be reported later in the process.
cb = fdst->cb; cbopaque = fdst->opaque; ff = fdst->ff; @@ -787,10 +790,10 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) if (fdst->thread) { char *buf;
- if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { virReportSystemError(EBADF, "%s", _("cannot write to stream"));
Would this (and the subsequent similar check) possibly overwrite something from a threadErr with this error message?
What do you mean? In case of thread error, threadErr is set to errno. So we should do: virReportSystemError(fdst->threadErr, ...); instead of EBADF? Well, look for the very next patch.
- return -1; + goto cleanup;
ouch, <sigh> and this is separable too technically....
Yes, technically. On the other hand, how strictly do we want to follow the rules? ;-)
}
if (VIR_ALLOC(msg) < 0 || @@ -866,7 +869,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) virFDStreamMsgPtr msg = NULL;
while (!(msg = fdst->msg)) { - if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { if (nbytes) { virReportSystemError(EBADF, "%s", _("stream is not open")); @@ -959,6 +962,9 @@ virFDStreamSendHole(virStreamPtr st, fdst->offset += length; }
+ if (fdst->threadErr) + goto cleanup; +
So it's interesting in these paths the threadErr check is done outside the fdst->thread check which is different than the virFDStreamRead and Write API's.
Well, if there's no fdst->thread then no one sets the threadErr. But okay, for consistency purpose I can move this check.
if (fdst->thread) { /* Things are a bit complicated here. If FDStream is in a * read mode, then if the message at the queue head is @@ -1018,6 +1024,9 @@ virFDStreamInData(virStreamPtr st,
virObjectLock(fdst);
+ if (fdst->threadErr) + goto cleanup; +
This one in particular because threadQuit is checked in the subsequent code similar to virFDStreamRead made me ponder a bit more, but I'm not sure I could come to a conclusion...
This is slightly different. I mean, I can move the check, but this is sort of a special API in sense that if the I/O thread exits and the msg queue is empty we will signal EOF to the caller. The other three APIs do change state of the stream - they change the position in the stream. This is a query API and as such can be used to query "are we at EOF yet?". That is the reason why this API doesn't fail on threadQuit, rather than return EOF (in a specific way as described in virStreamInData description).
I guess I'd want to see some consistency over when threadErr is checked between the 4 functions or I'd wonder why 2 do things one way and 2 the other. If they need to be different for a specific reason, then I suppose more separation could be done or at least the commit message indicate why they're different.
Okay. I can be more verbose there. Michal
On 06/13/2017 07:31 AM, Michal Privoznik wrote:
On 06/12/2017 11:26 PM, John Ferlan wrote:
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
When the I/O thread quits (e.g. due to an I/O error, lseek() error, whatever), any subsequent virFDStream API should return error too. Moreover, when invoking stream event callback, we must set the VIR_STREAM_EVENT_ERROR flag so that the callback knows something bad happened.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfdstream.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 7ee58be13..cd24757e6 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -312,6 +312,9 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; }
+ if (fdst->threadErr) + events |= VIR_STREAM_EVENT_ERROR; +
This hunk almost feels separable... That is, it's updating the events (VIR_STREAM_EVENT_ERROR) in the callback function feels different than checking for threadErr more often, but I'll leave it up to you to decide to split more or not.
I don't think that separation is needed. The commit says "Check for thread error more frequently". Every check comes from an action. In majority of the cases the action is reporting an error. In some cases it means setting a flag, a boolean that causes error to be reported later in the process.
"almost" - hand grenades, atomic warfare, etc. ;-) I'm fine with keeping as is.
cb = fdst->cb; cbopaque = fdst->opaque; ff = fdst->ff; @@ -787,10 +790,10 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) if (fdst->thread) { char *buf;
- if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { virReportSystemError(EBADF, "%s", _("cannot write to stream"));
Would this (and the subsequent similar check) possibly overwrite something from a threadErr with this error message?
What do you mean? In case of thread error, threadErr is set to errno. So we should do:
virReportSystemError(fdst->threadErr, ...);
instead of EBADF? Well, look for the very next patch.
As you saw in the next patch, it was too easy for me to be lost in the weeds of which error is which... The comment was more towards how virFDStreamThread would set threadErr to errno after: 1. virCondWait fails in which case a virReportSystemError was issued 2. if (got < 0) on *DoRead and *DoWrite where I didn't chase throughout the calls, but there were virReportSystemError calls already made. so the question in my mind was on the '|| fdst->threadErr' would a message already be generated? or is the error message for ->thread thread as opposed to this processing thread?
- return -1; + goto cleanup;
ouch, <sigh> and this is separable too technically....
Yes, technically. On the other hand, how strictly do we want to follow the rules? ;-)
Rules, we don't need no stinking rules ;-)... I would have been remiss if I didn't point it out though. I'm fine with it as is since it's not like this is being backported somewhere special to fix a specific problem noted in some rogue QE test.
}
if (VIR_ALLOC(msg) < 0 || @@ -866,7 +869,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) virFDStreamMsgPtr msg = NULL;
while (!(msg = fdst->msg)) { - if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { if (nbytes) { virReportSystemError(EBADF, "%s", _("stream is not open")); @@ -959,6 +962,9 @@ virFDStreamSendHole(virStreamPtr st, fdst->offset += length; }
+ if (fdst->threadErr) + goto cleanup; +
So it's interesting in these paths the threadErr check is done outside the fdst->thread check which is different than the virFDStreamRead and Write API's.
Well, if there's no fdst->thread then no one sets the threadErr. But okay, for consistency purpose I can move this check.
The question that popped into my mind was at this point was why not alter the previous calls to make the check earlier. Especially considering that both of the previous ones also potentially calls virReportSystemError. For this particular API, I agree it's not an issue here, but I was going on the consistency route with the thought in the back of my mind about someone looking at this code at some point in the future and wondering why the checks were being made at different places.
if (fdst->thread) { /* Things are a bit complicated here. If FDStream is in a * read mode, then if the message at the queue head is @@ -1018,6 +1024,9 @@ virFDStreamInData(virStreamPtr st,
virObjectLock(fdst);
+ if (fdst->threadErr) + goto cleanup; +
This one in particular because threadQuit is checked in the subsequent code similar to virFDStreamRead made me ponder a bit more, but I'm not sure I could come to a conclusion...
This is slightly different. I mean, I can move the check, but this is sort of a special API in sense that if the I/O thread exits and the msg queue is empty we will signal EOF to the caller. The other three APIs do change state of the stream - they change the position in the stream. This is a query API and as such can be used to query "are we at EOF yet?". That is the reason why this API doesn't fail on threadQuit, rather than return EOF (in a specific way as described in virStreamInData description).
Makes sense when you consider the totality of things. The new API's treat threadErr as different than threadQuit, while the old API's treat them the same which is what I was trying to consider. I kept going back to that error message being generated for the earlier two calls and wondering what, if any, downside there is to doing that. John
I guess I'd want to see some consistency over when threadErr is checked between the 4 functions or I'd wonder why 2 do things one way and 2 the other. If they need to be different for a specific reason, then I suppose more separation could be done or at least the commit message indicate why they're different.
Okay. I can be more verbose there.
Michal
On 06/13/2017 02:35 PM, John Ferlan wrote:
On 06/13/2017 07:31 AM, Michal Privoznik wrote:
On 06/12/2017 11:26 PM, John Ferlan wrote:
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
When the I/O thread quits (e.g. due to an I/O error, lseek() error, whatever), any subsequent virFDStream API should return error too. Moreover, when invoking stream event callback, we must set the VIR_STREAM_EVENT_ERROR flag so that the callback knows something bad happened.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfdstream.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 7ee58be13..cd24757e6 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -312,6 +312,9 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; }
+ if (fdst->threadErr) + events |= VIR_STREAM_EVENT_ERROR; +
This hunk almost feels separable... That is, it's updating the events (VIR_STREAM_EVENT_ERROR) in the callback function feels different than checking for threadErr more often, but I'll leave it up to you to decide to split more or not.
I don't think that separation is needed. The commit says "Check for thread error more frequently". Every check comes from an action. In majority of the cases the action is reporting an error. In some cases it means setting a flag, a boolean that causes error to be reported later in the process.
"almost" - hand grenades, atomic warfare, etc. ;-)
I'm fine with keeping as is.
cb = fdst->cb; cbopaque = fdst->opaque; ff = fdst->ff; @@ -787,10 +790,10 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) if (fdst->thread) { char *buf;
- if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { virReportSystemError(EBADF, "%s", _("cannot write to stream"));
Would this (and the subsequent similar check) possibly overwrite something from a threadErr with this error message?
What do you mean? In case of thread error, threadErr is set to errno. So we should do:
virReportSystemError(fdst->threadErr, ...);
instead of EBADF? Well, look for the very next patch.
As you saw in the next patch, it was too easy for me to be lost in the weeds of which error is which...
The comment was more towards how virFDStreamThread would set threadErr to errno after:
1. virCondWait fails in which case a virReportSystemError was issued 2. if (got < 0) on *DoRead and *DoWrite where I didn't chase throughout the calls, but there were virReportSystemError calls already made.
so the question in my mind was on the '|| fdst->threadErr' would a message already be generated?
Yes.
or is the error message for ->thread thread as opposed to this processing thread?
What do you mean?
if (fdst->thread) { /* Things are a bit complicated here. If FDStream is in a * read mode, then if the message at the queue head is @@ -1018,6 +1024,9 @@ virFDStreamInData(virStreamPtr st,
virObjectLock(fdst);
+ if (fdst->threadErr) + goto cleanup; +
This one in particular because threadQuit is checked in the subsequent code similar to virFDStreamRead made me ponder a bit more, but I'm not sure I could come to a conclusion...
This is slightly different. I mean, I can move the check, but this is sort of a special API in sense that if the I/O thread exits and the msg queue is empty we will signal EOF to the caller. The other three APIs do change state of the stream - they change the position in the stream. This is a query API and as such can be used to query "are we at EOF yet?". That is the reason why this API doesn't fail on threadQuit, rather than return EOF (in a specific way as described in virStreamInData description).
Makes sense when you consider the totality of things. The new API's treat threadErr as different than threadQuit, while the old API's treat them the same which is what I was trying to consider.
I kept going back to that error message being generated for the earlier two calls and wondering what, if any, downside there is to doing that.
There is none. It's just that I can use already existing piece of code and append my new check into it. Michal
On 06/13/2017 09:26 AM, Michal Privoznik wrote:
On 06/13/2017 02:35 PM, John Ferlan wrote:
On 06/13/2017 07:31 AM, Michal Privoznik wrote:
On 06/12/2017 11:26 PM, John Ferlan wrote:
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
When the I/O thread quits (e.g. due to an I/O error, lseek() error, whatever), any subsequent virFDStream API should return error too. Moreover, when invoking stream event callback, we must set the VIR_STREAM_EVENT_ERROR flag so that the callback knows something bad happened.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfdstream.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 7ee58be13..cd24757e6 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -312,6 +312,9 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; }
+ if (fdst->threadErr) + events |= VIR_STREAM_EVENT_ERROR; +
This hunk almost feels separable... That is, it's updating the events (VIR_STREAM_EVENT_ERROR) in the callback function feels different than checking for threadErr more often, but I'll leave it up to you to decide to split more or not.
I don't think that separation is needed. The commit says "Check for thread error more frequently". Every check comes from an action. In majority of the cases the action is reporting an error. In some cases it means setting a flag, a boolean that causes error to be reported later in the process.
"almost" - hand grenades, atomic warfare, etc. ;-)
I'm fine with keeping as is.
cb = fdst->cb; cbopaque = fdst->opaque; ff = fdst->ff; @@ -787,10 +790,10 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) if (fdst->thread) { char *buf;
- if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { virReportSystemError(EBADF, "%s", _("cannot write to stream"));
Would this (and the subsequent similar check) possibly overwrite something from a threadErr with this error message?
What do you mean? In case of thread error, threadErr is set to errno. So we should do:
virReportSystemError(fdst->threadErr, ...);
instead of EBADF? Well, look for the very next patch.
As you saw in the next patch, it was too easy for me to be lost in the weeds of which error is which...
The comment was more towards how virFDStreamThread would set threadErr to errno after:
1. virCondWait fails in which case a virReportSystemError was issued 2. if (got < 0) on *DoRead and *DoWrite where I didn't chase throughout the calls, but there were virReportSystemError calls already made.
so the question in my mind was on the '|| fdst->threadErr' would a message already be generated?
Yes.
In which case, then wouldn't this message overwrite the one already there. Is that what we want "at this point". I guess it's always been in the back of my mind that we want to display the specific message that caused the original error rather than a perhaps more generic message from some calling function. It seems we have algorithms that take special care not to overwrite some pre-existing error message, but wouldn't this path/change now generate for display the more generic message. Of course you probably have patch2 in mind where you grab the error message from the other thread and overwrite the "last message" so what gets displayed is that error instead of "cannot write to stream".
or is the error message for ->thread thread as opposed to this processing thread?
What do you mean?
Getting lost in the weeds of multiple patches while trying to consider the singularity of "this" patch. I think after patch 2 what I'm thinking about won't matter since whatever message caused fdst->thread to fail will be set, true?
if (fdst->thread) { /* Things are a bit complicated here. If FDStream is in a * read mode, then if the message at the queue head is @@ -1018,6 +1024,9 @@ virFDStreamInData(virStreamPtr st,
virObjectLock(fdst);
+ if (fdst->threadErr) + goto cleanup; +
This one in particular because threadQuit is checked in the subsequent code similar to virFDStreamRead made me ponder a bit more, but I'm not sure I could come to a conclusion...
This is slightly different. I mean, I can move the check, but this is sort of a special API in sense that if the I/O thread exits and the msg queue is empty we will signal EOF to the caller. The other three APIs do change state of the stream - they change the position in the stream. This is a query API and as such can be used to query "are we at EOF yet?". That is the reason why this API doesn't fail on threadQuit, rather than return EOF (in a specific way as described in virStreamInData description).
Makes sense when you consider the totality of things. The new API's treat threadErr as different than threadQuit, while the old API's treat them the same which is what I was trying to consider.
I kept going back to that error message being generated for the earlier two calls and wondering what, if any, downside there is to doing that.
There is none. It's just that I can use already existing piece of code and append my new check into it.
Michal
While yes, error messages get logged somewhere, isn't it only the "last" one that gets displayed to/for the "user"? I think that's what I'm hung up on. John
Problem with our error reporting is that the error object is a thread local variable. That means if there's an error reported within the I/O thread it gets logged and everything, but later when the event loop aborts the stream it doesn't see the original error. So we are left with some generic error. We can do better if we copy the error message between the threads. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/stream.c | 18 ++++++++++++------ src/util/virfdstream.c | 9 ++++++--- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/daemon/stream.c b/daemon/stream.c index 1d5b50ad7..5077ac8b0 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -231,17 +231,23 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) int ret; virNetMessagePtr msg; virNetMessageError rerr; + virErrorPtr origErr = virSaveLastError(); memset(&rerr, 0, sizeof(rerr)); stream->closed = true; virStreamEventRemoveCallback(stream->st); virStreamAbort(stream->st); - if (events & VIR_STREAM_EVENT_HANGUP) - virReportError(VIR_ERR_RPC, - "%s", _("stream had unexpected termination")); - else - virReportError(VIR_ERR_RPC, - "%s", _("stream had I/O failure")); + if (origErr && origErr->code != VIR_ERR_OK) { + virSetError(origErr); + virFreeError(origErr); + } else { + if (events & VIR_STREAM_EVENT_HANGUP) + virReportError(VIR_ERR_RPC, + "%s", _("stream had unexpected termination")); + else + virReportError(VIR_ERR_RPC, + "%s", _("stream had I/O failure")); + } msg = virNetMessageNew(false); if (!msg) { diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index cd24757e6..5b59765fa 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -106,7 +106,7 @@ struct virFDStreamData { /* Thread data */ virThreadPtr thread; virCond threadCond; - int threadErr; + virErrorPtr threadErr; bool threadQuit; bool threadAbort; bool threadDoRead; @@ -123,6 +123,7 @@ virFDStreamDataDispose(void *obj) virFDStreamDataPtr fdst = obj; VIR_DEBUG("obj=%p", fdst); + virFreeError(fdst->threadErr); virFDStreamMsgQueueFree(&fdst->msg); } @@ -312,8 +313,10 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; } - if (fdst->threadErr) + if (fdst->threadErr) { events |= VIR_STREAM_EVENT_ERROR; + virSetError(fdst->threadErr); + } cb = fdst->cb; cbopaque = fdst->opaque; @@ -637,7 +640,7 @@ virFDStreamThread(void *opaque) return; error: - fdst->threadErr = errno; + fdst->threadErr = virSaveLastError(); goto cleanup; } -- 2.13.0
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
Problem with our error reporting is that the error object is a thread local variable. That means if there's an error reported within the I/O thread it gets logged and everything, but later when the event loop aborts the stream it doesn't see the original error. So we are left with some generic error. We can do better if we copy the error message between the threads.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/stream.c | 18 ++++++++++++------ src/util/virfdstream.c | 9 ++++++--- 2 files changed, 18 insertions(+), 9 deletions(-)
Perhaps I'm lost in weeds of this code, but I thought the magic of error message details for the threads was handled through rerr (and virNetMessageSaveError). Still, based on later patches which seem to care about the value of errno, it interesting this one removes an errno setting and replaces it with an error message. Maybe the answer is rather than replacing threadErr it should be 'augmented' with a 'threadErrMsg'.
diff --git a/daemon/stream.c b/daemon/stream.c index 1d5b50ad7..5077ac8b0 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -231,17 +231,23 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) int ret; virNetMessagePtr msg; virNetMessageError rerr; + virErrorPtr origErr = virSaveLastError();
memset(&rerr, 0, sizeof(rerr)); stream->closed = true; virStreamEventRemoveCallback(stream->st); virStreamAbort(stream->st); - if (events & VIR_STREAM_EVENT_HANGUP) - virReportError(VIR_ERR_RPC, - "%s", _("stream had unexpected termination")); - else - virReportError(VIR_ERR_RPC, - "%s", _("stream had I/O failure")); + if (origErr && origErr->code != VIR_ERR_OK) { + virSetError(origErr); + virFreeError(origErr); + } else { + if (events & VIR_STREAM_EVENT_HANGUP) + virReportError(VIR_ERR_RPC, + "%s", _("stream had unexpected termination")); + else + virReportError(VIR_ERR_RPC, + "%s", _("stream had I/O failure")); + }
This seems fine - display the error that we got from some lower spot if it exists; otherwise, Might be important to note the saving of the error has a lot to do with virStreamAbort's clearing. At the very least this is an error in our processing vs. something within the stream, correct? Or is that the thicket of weeds I'm lost in.
msg = virNetMessageNew(false); if (!msg) { diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index cd24757e6..5b59765fa 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -106,7 +106,7 @@ struct virFDStreamData { /* Thread data */ virThreadPtr thread; virCond threadCond; - int threadErr; + virErrorPtr threadErr; bool threadQuit; bool threadAbort; bool threadDoRead; @@ -123,6 +123,7 @@ virFDStreamDataDispose(void *obj) virFDStreamDataPtr fdst = obj;
VIR_DEBUG("obj=%p", fdst); + virFreeError(fdst->threadErr); virFDStreamMsgQueueFree(&fdst->msg); }
@@ -312,8 +313,10 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; }
- if (fdst->threadErr) + if (fdst->threadErr) { events |= VIR_STREAM_EVENT_ERROR; + virSetError(fdst->threadErr);> + }
cb = fdst->cb; cbopaque = fdst->opaque; @@ -637,7 +640,7 @@ virFDStreamThread(void *opaque) return;
error: - fdst->threadErr = errno; + fdst->threadErr = virSaveLastError();
"Typically" the processing is: orig_err = virSaveLastError(); { do stuff }; if (orig_err) { virSetError(orig_err); virFreeError(orig_err); } So, how do we know threadErr got something in return and if it didn't we've messed up "other" algorithms. In particular, the lines in the previous hunk won't set 'events' any more. I'm not worried about being called twice (we know that won't happen ;-)), but spreading out the error message processing through 3 functions causes me wonder whether that virFreeError should be done right after the virSetError rather than waiting for virFDStreamDataDispose. John
goto cleanup; }
On 06/12/2017 11:48 PM, John Ferlan wrote:
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
Problem with our error reporting is that the error object is a thread local variable. That means if there's an error reported within the I/O thread it gets logged and everything, but later when the event loop aborts the stream it doesn't see the original error. So we are left with some generic error. We can do better if we copy the error message between the threads.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/stream.c | 18 ++++++++++++------ src/util/virfdstream.c | 9 ++++++--- 2 files changed, 18 insertions(+), 9 deletions(-)
Perhaps I'm lost in weeds of this code, but I thought the magic of error message details for the threads was handled through rerr (and virNetMessageSaveError).
Not really. When you call virReportError()/virReportSystemError() these functions: 1) fetch thread local error object 2) if it doesn't exist they create one 3) copy the error message and couple of other things into the object Therefore, every thread has its own error object and unless two threads share a pointer, they cannot see each others error object. Now, we have two threads when it comes to server side of streams: the event loop (which sends data to the client), and I/O thread. Should something go wrong in the latter, we need to save the error object somewhere so that the former can pick it up.
Still, based on later patches which seem to care about the value of errno, it interesting this one removes an errno setting and replaces it with an error message.
With a pointer. That way we can still check if it is set or not. Prior to this patch, threadErr is an int, and checks are: "if (!fdst->threadErr)". After that threadErr is a pointer so that the checks can stay the same.
Maybe the answer is rather than replacing threadErr it should be 'augmented' with a 'threadErrMsg'.
Why? Prior to this patch threadErr despite being type of was used as a boolean really. What would be the benefits of having both int threadErr; and virErrorPtr trheadErrMsg;?
diff --git a/daemon/stream.c b/daemon/stream.c index 1d5b50ad7..5077ac8b0 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -231,17 +231,23 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) int ret; virNetMessagePtr msg; virNetMessageError rerr; + virErrorPtr origErr = virSaveLastError();
memset(&rerr, 0, sizeof(rerr)); stream->closed = true; virStreamEventRemoveCallback(stream->st); virStreamAbort(stream->st); - if (events & VIR_STREAM_EVENT_HANGUP) - virReportError(VIR_ERR_RPC, - "%s", _("stream had unexpected termination")); - else - virReportError(VIR_ERR_RPC, - "%s", _("stream had I/O failure")); + if (origErr && origErr->code != VIR_ERR_OK) { + virSetError(origErr); + virFreeError(origErr); + } else { + if (events & VIR_STREAM_EVENT_HANGUP) + virReportError(VIR_ERR_RPC, + "%s", _("stream had unexpected termination")); + else + virReportError(VIR_ERR_RPC, + "%s", _("stream had I/O failure")); + }
This seems fine - display the error that we got from some lower spot if it exists; otherwise,
Might be important to note the saving of the error has a lot to do with virStreamAbort's clearing.
Well, we don't have comment on other places like this, but okay, I can add a comment that says "save original error in case something resets its later", or something among those lines.
At the very least this is an error in our processing vs. something within the stream, correct? Or is that the thicket of weeds I'm lost in.
msg = virNetMessageNew(false); if (!msg) { diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index cd24757e6..5b59765fa 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -106,7 +106,7 @@ struct virFDStreamData { /* Thread data */ virThreadPtr thread; virCond threadCond; - int threadErr; + virErrorPtr threadErr; bool threadQuit; bool threadAbort; bool threadDoRead; @@ -123,6 +123,7 @@ virFDStreamDataDispose(void *obj) virFDStreamDataPtr fdst = obj;
VIR_DEBUG("obj=%p", fdst); + virFreeError(fdst->threadErr); virFDStreamMsgQueueFree(&fdst->msg); }
@@ -312,8 +313,10 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; }
- if (fdst->threadErr) + if (fdst->threadErr) { events |= VIR_STREAM_EVENT_ERROR; + virSetError(fdst->threadErr);> + }
cb = fdst->cb; cbopaque = fdst->opaque; @@ -637,7 +640,7 @@ virFDStreamThread(void *opaque) return;
error: - fdst->threadErr = errno; + fdst->threadErr = virSaveLastError();
"Typically" the processing is:
orig_err = virSaveLastError(); { do stuff }; if (orig_err) { virSetError(orig_err); virFreeError(orig_err); }
This is not typical. This hunk here (virSaveLastError()) is called from within the I/O thread. And as I've explained earlier, error objects are thread local. So what this hunk does is save any error reported earlier so that the other hunk above (virSetError()) can set it (it runs in a different threat). What's not so typical about this? I don't think that we copy error objects between two threads anywhere else in the code.
So, how do we know threadErr got something in return and if it didn't we've messed up "other" algorithms. In particular, the lines in the previous hunk won't set 'events' any more.
How come? threadErr is set if and only if the I/O thread finished with an error.
I'm not worried about being called twice (we know that won't happen ;-)), but spreading out the error message processing through 3 functions causes me wonder whether that virFreeError should be done right after the virSetError rather than waiting for virFDStreamDataDispose.
I don't think so. threadError is part of virFDStreamData object and as such the dispose function is responsible for freeing the object. Michal
On 06/13/2017 07:31 AM, Michal Privoznik wrote:
On 06/12/2017 11:48 PM, John Ferlan wrote:
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
Problem with our error reporting is that the error object is a thread local variable. That means if there's an error reported within the I/O thread it gets logged and everything, but later when the event loop aborts the stream it doesn't see the original error. So we are left with some generic error. We can do better if we copy the error message between the threads.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/stream.c | 18 ++++++++++++------ src/util/virfdstream.c | 9 ++++++--- 2 files changed, 18 insertions(+), 9 deletions(-)
Perhaps I'm lost in weeds of this code, but I thought the magic of error message details for the threads was handled through rerr (and virNetMessageSaveError).
Not really. When you call virReportError()/virReportSystemError() these functions:
1) fetch thread local error object 2) if it doesn't exist they create one 3) copy the error message and couple of other things into the object
Therefore, every thread has its own error object and unless two threads share a pointer, they cannot see each others error object. Now, we have two threads when it comes to server side of streams: the event loop (which sends data to the client), and I/O thread. Should something go wrong in the latter, we need to save the error object somewhere so that the former can pick it up.
Right, I'm running through the weeds of error messages.
Still, based on later patches which seem to care about the value of errno, it interesting this one removes an errno setting and replaces it with an error message.
With a pointer. That way we can still check if it is set or not. Prior to this patch, threadErr is an int, and checks are: "if (!fdst->threadErr)". After that threadErr is a pointer so that the checks can stay the same.
Right because coding practices have always been that if "(!0)" is OK; whereas, the check could have been "if (fdst->threadErr != 0)".
Maybe the answer is rather than replacing threadErr it should be 'augmented' with a 'threadErrMsg'.
Why? Prior to this patch threadErr despite being type of was used as a boolean really. What would be the benefits of having both int threadErr; and virErrorPtr trheadErrMsg;?
I read patch6 and started down the path of thinking which thread am I in and if saving/fetching errno was important for those callback functions - would it be something necessary to save in the fdst as well in addition to saving the last error message. Then of course the what if virSaveLastError returned NULL for some reason in virFDStreamThread, but errno was set. Considering all the possibilities.
diff --git a/daemon/stream.c b/daemon/stream.c index 1d5b50ad7..5077ac8b0 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -231,17 +231,23 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) int ret; virNetMessagePtr msg; virNetMessageError rerr; + virErrorPtr origErr = virSaveLastError();
memset(&rerr, 0, sizeof(rerr)); stream->closed = true; virStreamEventRemoveCallback(stream->st); virStreamAbort(stream->st); - if (events & VIR_STREAM_EVENT_HANGUP) - virReportError(VIR_ERR_RPC, - "%s", _("stream had unexpected termination")); - else - virReportError(VIR_ERR_RPC, - "%s", _("stream had I/O failure")); + if (origErr && origErr->code != VIR_ERR_OK) { + virSetError(origErr); + virFreeError(origErr); + } else { + if (events & VIR_STREAM_EVENT_HANGUP) + virReportError(VIR_ERR_RPC, + "%s", _("stream had unexpected termination")); + else + virReportError(VIR_ERR_RPC, + "%s", _("stream had I/O failure")); + }
This seems fine - display the error that we got from some lower spot if it exists; otherwise,
Might be important to note the saving of the error has a lot to do with virStreamAbort's clearing.
Well, we don't have comment on other places like this, but okay, I can add a comment that says "save original error in case something resets its later", or something among those lines.
A commit message noting would be fine. This only pops into my consciousness because of patch 4.
At the very least this is an error in our processing vs. something within the stream, correct? Or is that the thicket of weeds I'm lost in.
msg = virNetMessageNew(false); if (!msg) { diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index cd24757e6..5b59765fa 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -106,7 +106,7 @@ struct virFDStreamData { /* Thread data */ virThreadPtr thread; virCond threadCond; - int threadErr; + virErrorPtr threadErr; bool threadQuit; bool threadAbort; bool threadDoRead; @@ -123,6 +123,7 @@ virFDStreamDataDispose(void *obj) virFDStreamDataPtr fdst = obj;
VIR_DEBUG("obj=%p", fdst); + virFreeError(fdst->threadErr); virFDStreamMsgQueueFree(&fdst->msg); }
@@ -312,8 +313,10 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; }
- if (fdst->threadErr) + if (fdst->threadErr) { events |= VIR_STREAM_EVENT_ERROR; + virSetError(fdst->threadErr);> + }
cb = fdst->cb; cbopaque = fdst->opaque; @@ -637,7 +640,7 @@ virFDStreamThread(void *opaque) return;
error: - fdst->threadErr = errno; + fdst->threadErr = virSaveLastError();
"Typically" the processing is:
orig_err = virSaveLastError(); { do stuff }; if (orig_err) { virSetError(orig_err); virFreeError(orig_err); }
This is not typical.
agreed!
This hunk here (virSaveLastError()) is called from within the I/O thread. And as I've explained earlier, error objects are thread local. So what this hunk does is save any error reported earlier so that the other hunk above (virSetError()) can set it (it runs in a different threat). What's not so typical about this? I don't think that we copy error objects between two threads anywhere else in the code.
The only place I would think we could do that would be somewhere in migration code, vmagent code, or perhaps some qemu driver algorithm dealing with stats or snapshots. I didn't look.
So, how do we know threadErr got something in return and if it didn't we've messed up "other" algorithms. In particular, the lines in the previous hunk won't set 'events' any more.
How come? threadErr is set if and only if the I/O thread finished with an error.
virSaveLastError can return NULL even though @errno was non-zero, so even though in that instance we're not going to get much more done - it does mean that code that was going to fail previously because errno had been set will now succeed at least for a short while rather than causing an error like it should have.
I'm not worried about being called twice (we know that won't happen ;-)), but spreading out the error message processing through 3 functions causes me wonder whether that virFreeError should be done right after the virSetError rather than waiting for virFDStreamDataDispose.
I don't think so. threadError is part of virFDStreamData object and as such the dispose function is responsible for freeing the object.
Michal
So threadA has an error message allocated that now gets set or copied into threadB. So, virFDStreamDataDispose is running for threadA, right? That's where I'm lost in the thicket. Still, as I'm typing this I'm thinking if we Free'd it right after the Set, then we'd have to set threadErr = NULL too and that causes other algorithm problems possibly. OK - so I've convinced myself that it's better in Dispose! John
On 06/13/2017 03:13 PM, John Ferlan wrote:
On 06/13/2017 07:31 AM, Michal Privoznik wrote:
On 06/12/2017 11:48 PM, John Ferlan wrote:
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
Problem with our error reporting is that the error object is a thread local variable. That means if there's an error reported within the I/O thread it gets logged and everything, but later when the event loop aborts the stream it doesn't see the original error. So we are left with some generic error. We can do better if we copy the error message between the threads.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/stream.c | 18 ++++++++++++------ src/util/virfdstream.c | 9 ++++++--- 2 files changed, 18 insertions(+), 9 deletions(-)
Perhaps I'm lost in weeds of this code, but I thought the magic of error message details for the threads was handled through rerr (and virNetMessageSaveError).
Not really. When you call virReportError()/virReportSystemError() these functions:
1) fetch thread local error object 2) if it doesn't exist they create one 3) copy the error message and couple of other things into the object
Therefore, every thread has its own error object and unless two threads share a pointer, they cannot see each others error object. Now, we have two threads when it comes to server side of streams: the event loop (which sends data to the client), and I/O thread. Should something go wrong in the latter, we need to save the error object somewhere so that the former can pick it up.
Right, I'm running through the weeds of error messages.
Still, based on later patches which seem to care about the value of errno, it interesting this one removes an errno setting and replaces it with an error message.
With a pointer. That way we can still check if it is set or not. Prior to this patch, threadErr is an int, and checks are: "if (!fdst->threadErr)". After that threadErr is a pointer so that the checks can stay the same.
Right because coding practices have always been that if "(!0)" is OK; whereas, the check could have been "if (fdst->threadErr != 0)".
Maybe the answer is rather than replacing threadErr it should be 'augmented' with a 'threadErrMsg'.
Why? Prior to this patch threadErr despite being type of was used as a boolean really. What would be the benefits of having both int threadErr; and virErrorPtr trheadErrMsg;?
I read patch6 and started down the path of thinking which thread am I in and if saving/fetching errno was important for those callback functions - would it be something necessary to save in the fdst as well in addition to saving the last error message.
That is something different. Errno set by callbacks from patch 6 is on the client side. The callbacks are used exclusively by client as it only uses StreamRecvAll/StreamSendAll wrappers. This errno is set on the server side by the I/O thread.
Then of course the what if virSaveLastError returned NULL for some reason in virFDStreamThread, but errno was set. Considering all the possibilities.
Well, the only case when virSaveLastError can return NULL is OOM. True, we will not see it here, but something else will for sure. Then again, on Linux you won't see malloc() failing. The program gets terminated instead.
diff --git a/daemon/stream.c b/daemon/stream.c index 1d5b50ad7..5077ac8b0 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -231,17 +231,23 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) int ret; virNetMessagePtr msg; virNetMessageError rerr; + virErrorPtr origErr = virSaveLastError();
memset(&rerr, 0, sizeof(rerr)); stream->closed = true; virStreamEventRemoveCallback(stream->st); virStreamAbort(stream->st); - if (events & VIR_STREAM_EVENT_HANGUP) - virReportError(VIR_ERR_RPC, - "%s", _("stream had unexpected termination")); - else - virReportError(VIR_ERR_RPC, - "%s", _("stream had I/O failure")); + if (origErr && origErr->code != VIR_ERR_OK) { + virSetError(origErr); + virFreeError(origErr); + } else { + if (events & VIR_STREAM_EVENT_HANGUP) + virReportError(VIR_ERR_RPC, + "%s", _("stream had unexpected termination")); + else + virReportError(VIR_ERR_RPC, + "%s", _("stream had I/O failure")); + }
This seems fine - display the error that we got from some lower spot if it exists; otherwise,
Might be important to note the saving of the error has a lot to do with virStreamAbort's clearing.
Well, we don't have comment on other places like this, but okay, I can add a comment that says "save original error in case something resets its later", or something among those lines.
A commit message noting would be fine. This only pops into my consciousness because of patch 4.
At the very least this is an error in our processing vs. something within the stream, correct? Or is that the thicket of weeds I'm lost in.
msg = virNetMessageNew(false); if (!msg) { diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index cd24757e6..5b59765fa 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -106,7 +106,7 @@ struct virFDStreamData { /* Thread data */ virThreadPtr thread; virCond threadCond; - int threadErr; + virErrorPtr threadErr; bool threadQuit; bool threadAbort; bool threadDoRead; @@ -123,6 +123,7 @@ virFDStreamDataDispose(void *obj) virFDStreamDataPtr fdst = obj;
VIR_DEBUG("obj=%p", fdst); + virFreeError(fdst->threadErr); virFDStreamMsgQueueFree(&fdst->msg); }
@@ -312,8 +313,10 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; }
- if (fdst->threadErr) + if (fdst->threadErr) { events |= VIR_STREAM_EVENT_ERROR; + virSetError(fdst->threadErr);> + }
cb = fdst->cb; cbopaque = fdst->opaque; @@ -637,7 +640,7 @@ virFDStreamThread(void *opaque) return;
error: - fdst->threadErr = errno; + fdst->threadErr = virSaveLastError();
"Typically" the processing is:
orig_err = virSaveLastError(); { do stuff }; if (orig_err) { virSetError(orig_err); virFreeError(orig_err); }
This is not typical.
agreed!
This hunk here (virSaveLastError()) is called from within the I/O thread. And as I've explained earlier, error objects are thread local. So what this hunk does is save any error reported earlier so that the other hunk above (virSetError()) can set it (it runs in a different threat). What's not so typical about this? I don't think that we copy error objects between two threads anywhere else in the code.
The only place I would think we could do that would be somewhere in migration code, vmagent code, or perhaps some qemu driver algorithm dealing with stats or snapshots. I didn't look.
Yeah, maybe. But I haven't touched those areas in a while, therefore I think what I think - we don't do it anywhere :-)
So, how do we know threadErr got something in return and if it didn't we've messed up "other" algorithms. In particular, the lines in the previous hunk won't set 'events' any more.
How come? threadErr is set if and only if the I/O thread finished with an error.
virSaveLastError can return NULL even though @errno was non-zero,
Again, this can happen only on OOM in which case libvirtd is going to be killed anyway.
so even though in that instance we're not going to get much more done - it does mean that code that was going to fail previously because errno had been set will now succeed at least for a short while rather than causing an error like it should have.
Succeeding for a short period of time it takes oom_killer to kill a process ;-)
I'm not worried about being called twice (we know that won't happen ;-)), but spreading out the error message processing through 3 functions causes me wonder whether that virFreeError should be done right after the virSetError rather than waiting for virFDStreamDataDispose.
I don't think so. threadError is part of virFDStreamData object and as such the dispose function is responsible for freeing the object.
Michal
So threadA has an error message allocated that now gets set or copied into threadB. So, virFDStreamDataDispose is running for threadA, right? That's where I'm lost in the thicket. Still, as I'm typing this I'm thinking if we Free'd it right after the Set, then we'd have to set threadErr = NULL too and that causes other algorithm problems possibly.
OK - so I've convinced myself that it's better in Dispose!
Cool! Michal
Our documentation to all four virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll says that if these functions fail, virStreamAbort() is called. But that is not necessarily true. For instance all of these functions allocate a buffer to work with. If the allocation fails, no virStreamAbort() is called despite -1 being returned. It's the same story with argument sanity checks and a lot of other checks. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 49 ++++++++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index d7a8f5816..bff0a0571 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -596,10 +596,8 @@ virStreamSendAll(virStreamPtr stream, for (;;) { int got, offset = 0; got = (handler)(stream, bytes, want, opaque); - if (got < 0) { - virStreamAbort(stream); + if (got < 0) goto cleanup; - } if (got == 0) break; while (offset < got) { @@ -615,8 +613,10 @@ virStreamSendAll(virStreamPtr stream, cleanup: VIR_FREE(bytes); - if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + } return ret; } @@ -728,21 +728,16 @@ int virStreamSparseSendAll(virStreamPtr stream, const unsigned int skipFlags = 0; if (!dataLen) { - if (holeHandler(stream, &inData, §ionLen, opaque) < 0) { - virStreamAbort(stream); + if (holeHandler(stream, &inData, §ionLen, opaque) < 0) goto cleanup; - } if (!inData && sectionLen) { - if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) { - virStreamAbort(stream); + if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) goto cleanup; - } if (skipHandler(stream, sectionLen, opaque) < 0) { virReportSystemError(errno, "%s", _("unable to skip hole")); - virStreamAbort(stream); goto cleanup; } continue; @@ -755,10 +750,8 @@ int virStreamSparseSendAll(virStreamPtr stream, want = dataLen; got = (handler)(stream, bytes, want, opaque); - if (got < 0) { - virStreamAbort(stream); + if (got < 0) goto cleanup; - } if (got == 0) break; while (offset < got) { @@ -775,8 +768,10 @@ int virStreamSparseSendAll(virStreamPtr stream, cleanup: VIR_FREE(bytes); - if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + } return ret; } @@ -857,10 +852,8 @@ virStreamRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) { - virStreamAbort(stream); + if (done < 0) goto cleanup; - } offset += done; } } @@ -869,8 +862,10 @@ virStreamRecvAll(virStreamPtr stream, cleanup: VIR_FREE(bytes); - if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + } return ret; } @@ -963,15 +958,11 @@ virStreamSparseRecvAll(virStreamPtr stream, got = virStreamRecvFlags(stream, bytes, want, flags); if (got == -3) { - if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) { - virStreamAbort(stream); + if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) goto cleanup; - } - if (holeHandler(stream, holeLen, opaque) < 0) { - virStreamAbort(stream); + if (holeHandler(stream, holeLen, opaque) < 0) goto cleanup; - } continue; } else if (got < 0) { goto cleanup; @@ -981,10 +972,8 @@ virStreamSparseRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) { - virStreamAbort(stream); + if (done < 0) goto cleanup; - } offset += done; } } @@ -993,8 +982,10 @@ virStreamSparseRecvAll(virStreamPtr stream, cleanup: VIR_FREE(bytes); - if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + } return ret; } -- 2.13.0
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
Our documentation to all four virStreamRecvAll, virStreamSendAll,
s/all four/the/
virStreamSparseRecvAll, virStreamSparseSendAll says that if these
s/RecvAll, virStream/RecvAll, and virStream/ s/says that if these functions fail,/functions indicates that on failure/
functions fail, virStreamAbort() is called. But that is not necessarily true. For instance all of these functions allocate a buffer to work with. If the allocation fails, no virStreamAbort() is called despite -1 being returned. It's the same story with argument sanity checks and a lot of other checks.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 49 ++++++++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 29 deletions(-)
This patch particularly scares me because there are so many more paths that could now call virStreamAbort. Yes, I know that's the point, but nonetheless it's large leap of faith to only calling Abort as a result of some Send/Recv callback failure to calling Abort for the ancillary stuff surrounding or supporting those calls as well. Would there be a chance of multiple callers to virStreamAbort? It'd be nice if there were any other opinions on this. I'd lean towards saying looks good, but I'm also interested if there's any other opposing opinions or concerns. John
diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index d7a8f5816..bff0a0571 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -596,10 +596,8 @@ virStreamSendAll(virStreamPtr stream, for (;;) { int got, offset = 0; got = (handler)(stream, bytes, want, opaque); - if (got < 0) { - virStreamAbort(stream); + if (got < 0) goto cleanup; - } if (got == 0) break; while (offset < got) { @@ -615,8 +613,10 @@ virStreamSendAll(virStreamPtr stream, cleanup: VIR_FREE(bytes);
- if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + }
return ret; } @@ -728,21 +728,16 @@ int virStreamSparseSendAll(virStreamPtr stream, const unsigned int skipFlags = 0;
if (!dataLen) { - if (holeHandler(stream, &inData, §ionLen, opaque) < 0) { - virStreamAbort(stream); + if (holeHandler(stream, &inData, §ionLen, opaque) < 0) goto cleanup; - }
if (!inData && sectionLen) { - if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) { - virStreamAbort(stream); + if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) goto cleanup; - }
if (skipHandler(stream, sectionLen, opaque) < 0) { virReportSystemError(errno, "%s", _("unable to skip hole")); - virStreamAbort(stream); goto cleanup; } continue; @@ -755,10 +750,8 @@ int virStreamSparseSendAll(virStreamPtr stream, want = dataLen;
got = (handler)(stream, bytes, want, opaque); - if (got < 0) { - virStreamAbort(stream); + if (got < 0) goto cleanup; - } if (got == 0) break; while (offset < got) { @@ -775,8 +768,10 @@ int virStreamSparseSendAll(virStreamPtr stream, cleanup: VIR_FREE(bytes);
- if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + }
return ret; } @@ -857,10 +852,8 @@ virStreamRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) { - virStreamAbort(stream); + if (done < 0) goto cleanup; - } offset += done; } } @@ -869,8 +862,10 @@ virStreamRecvAll(virStreamPtr stream, cleanup: VIR_FREE(bytes);
- if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + }
return ret; } @@ -963,15 +958,11 @@ virStreamSparseRecvAll(virStreamPtr stream,
got = virStreamRecvFlags(stream, bytes, want, flags); if (got == -3) { - if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) { - virStreamAbort(stream); + if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) goto cleanup; - }
- if (holeHandler(stream, holeLen, opaque) < 0) { - virStreamAbort(stream); + if (holeHandler(stream, holeLen, opaque) < 0) goto cleanup; - } continue; } else if (got < 0) { goto cleanup; @@ -981,10 +972,8 @@ virStreamSparseRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) { - virStreamAbort(stream); + if (done < 0) goto cleanup; - } offset += done; } } @@ -993,8 +982,10 @@ virStreamSparseRecvAll(virStreamPtr stream, cleanup: VIR_FREE(bytes);
- if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + }
return ret; }
On 06/13/2017 12:45 AM, John Ferlan wrote:
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
Our documentation to all four virStreamRecvAll, virStreamSendAll,
s/all four/the/
virStreamSparseRecvAll, virStreamSparseSendAll says that if these
s/RecvAll, virStream/RecvAll, and virStream/
s/says that if these functions fail,/functions indicates that on failure/
functions fail, virStreamAbort() is called. But that is not necessarily true. For instance all of these functions allocate a buffer to work with. If the allocation fails, no virStreamAbort() is called despite -1 being returned. It's the same story with argument sanity checks and a lot of other checks.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 49 ++++++++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 29 deletions(-)
This patch particularly scares me because there are so many more paths that could now call virStreamAbort. Yes, I know that's the point
Exactly :-)
, but nonetheless it's large leap of faith to only calling Abort as a result of some Send/Recv callback failure to calling Abort for the ancillary stuff surrounding or supporting those calls as well.
Why is that? The documentation to this function says that if this function fails, virStreamAbort has been called. So in fact currently this function might fail without calling virStreamAbort.
Would there be a chance of multiple callers to virStreamAbort?
Not over the same stream. The server side doesn't use these SendAll/RecvAll wrappers. They are for client only. And as such client is responsible for not calling them from two separate threads at the same time.
It'd be nice if there were any other opinions on this. I'd lean towards saying looks good, but I'm also interested if there's any other opposing opinions or concerns.
John
Michal
On 06/13/2017 07:31 AM, Michal Privoznik wrote:
On 06/13/2017 12:45 AM, John Ferlan wrote:
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
Our documentation to all four virStreamRecvAll, virStreamSendAll,
s/all four/the/
virStreamSparseRecvAll, virStreamSparseSendAll says that if these
s/RecvAll, virStream/RecvAll, and virStream/
s/says that if these functions fail,/functions indicates that on failure/
functions fail, virStreamAbort() is called. But that is not necessarily true. For instance all of these functions allocate a buffer to work with. If the allocation fails, no virStreamAbort() is called despite -1 being returned. It's the same story with argument sanity checks and a lot of other checks.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 49 ++++++++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 29 deletions(-)
This patch particularly scares me because there are so many more paths that could now call virStreamAbort. Yes, I know that's the point
Exactly :-)
, but nonetheless it's large leap of faith to only calling Abort as a result of some Send/Recv callback failure to calling Abort for the ancillary stuff surrounding or supporting those calls as well.
Why is that? The documentation to this function says that if this function fails, virStreamAbort has been called. So in fact currently this function might fail without calling virStreamAbort.
I understand/agree - it's still a large leap of faith though! It's always the nagging proposition of why was the code originally done the way it was where abort only happens in specific cases. Trying to peer back through the space and time continuum at commit id '182eba1b' and consider if there was a specific reason the "other" paths that could return -1 didn't call virStreamAbort? Why was it *only* the (handler) paths. Beyond the setting for the NONBLOCK and VIR_ALLOC_N failures, was the the expectation that somewhere in the virStreamSend/virStreamRecv calls that virStreamAbort would have been called on failure. Both note that the stream will be marked as aborted. Another "irony" is that the comment code example for virStreamSend shows the need to call virStreamAbort after a virStreamSend failure, but the same is not true for virStreamRecv failure example. IIUC, the SendAll/RecvAll are merely an implementation of those comments. So I'm left with that nagging doubt regarding why didn't the original implementation always ensure to call virStreamAbort if -1 was to be returned. I'm not against the change, I just was hoping someone else would be able to also speak up and say - yes, this is OK. Someone that perhaps has more detailed knowledge of the stream code than I do. Whether the code examples are updated in comments is entirely up to you, but if it helps someone else out in the future that'd be good especially since this is a "confusing" area. John
Would there be a chance of multiple callers to virStreamAbort?
Not over the same stream. The server side doesn't use these SendAll/RecvAll wrappers. They are for client only. And as such client is responsible for not calling them from two separate threads at the same time.
It'd be nice if there were any other opinions on this. I'd lean towards saying looks good, but I'm also interested if there's any other opposing opinions or concerns.
John
Michal
On 06/13/2017 03:41 PM, John Ferlan wrote:
On 06/13/2017 07:31 AM, Michal Privoznik wrote:
On 06/13/2017 12:45 AM, John Ferlan wrote:
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
Our documentation to all four virStreamRecvAll, virStreamSendAll,
s/all four/the/
virStreamSparseRecvAll, virStreamSparseSendAll says that if these
s/RecvAll, virStream/RecvAll, and virStream/
s/says that if these functions fail,/functions indicates that on failure/
functions fail, virStreamAbort() is called. But that is not necessarily true. For instance all of these functions allocate a buffer to work with. If the allocation fails, no virStreamAbort() is called despite -1 being returned. It's the same story with argument sanity checks and a lot of other checks.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 49 ++++++++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 29 deletions(-)
This patch particularly scares me because there are so many more paths that could now call virStreamAbort. Yes, I know that's the point
Exactly :-)
, but nonetheless it's large leap of faith to only calling Abort as a result of some Send/Recv callback failure to calling Abort for the ancillary stuff surrounding or supporting those calls as well.
Why is that? The documentation to this function says that if this function fails, virStreamAbort has been called. So in fact currently this function might fail without calling virStreamAbort.
I understand/agree - it's still a large leap of faith though! It's always the nagging proposition of why was the code originally done the way it was where abort only happens in specific cases. Trying to peer back through the space and time continuum at commit id '182eba1b' and consider if there was a specific reason the "other" paths that could return -1 didn't call virStreamAbort? Why was it *only* the (handler) paths.
Well. it's 2009. The code looked very differently then. Or maybe it jsut slipped review.
Beyond the setting for the NONBLOCK and VIR_ALLOC_N failures, was the the expectation that somewhere in the virStreamSend/virStreamRecv calls that virStreamAbort would have been called on failure. Both note that the stream will be marked as aborted.
Another "irony" is that the comment code example for virStreamSend shows the need to call virStreamAbort after a virStreamSend failure, but the same is not true for virStreamRecv failure example.
Ah, I haven't realized that! Sure. this definitely needs to be fixed too. In fact, it belongs to this patch IMO - since it's fixing the very same issue. Michal
If one these four functions fail (virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll) the stream is aborted by calling virStreamAbort(). This is, however, an public API - therefore the first thing it does is error reset. At that point any error that caused us to abort stream in the first place is gone. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index bff0a0571..1594ed212 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -614,7 +614,12 @@ virStreamSendAll(virStreamPtr stream, VIR_FREE(bytes); if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); } @@ -769,7 +774,12 @@ int virStreamSparseSendAll(virStreamPtr stream, VIR_FREE(bytes); if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); } @@ -863,7 +873,12 @@ virStreamRecvAll(virStreamPtr stream, VIR_FREE(bytes); if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); } @@ -983,7 +998,12 @@ virStreamSparseRecvAll(virStreamPtr stream, VIR_FREE(bytes); if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); } -- 2.13.0
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
If one these four functions fail (virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll) the stream is aborted by calling virStreamAbort(). This is, however, an public API - therefore the first thing it does is
s/This is, however, an/This is a/ s/ - therefore/; therefore,/
error reset. At that point any error that caused us to abort stream in the first place is gone.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
Almost makes me wonder why we don't have a local/static helper that will do the save, abort, restore rather than the repetition. Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index bff0a0571..1594ed212 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -614,7 +614,12 @@ virStreamSendAll(virStreamPtr stream, VIR_FREE(bytes);
if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); }
@@ -769,7 +774,12 @@ int virStreamSparseSendAll(virStreamPtr stream, VIR_FREE(bytes);
if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); }
@@ -863,7 +873,12 @@ virStreamRecvAll(virStreamPtr stream, VIR_FREE(bytes);
if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); }
@@ -983,7 +998,12 @@ virStreamSparseRecvAll(virStreamPtr stream, VIR_FREE(bytes);
if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); }
Callers might be interested in the original value of errno. Let's not overwrite it with lseek() done in cleanup path. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfile.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index d444b32f8..2be64f1db 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3900,8 +3900,11 @@ virFileInData(int fd, ret = 0; cleanup: /* At any rate, reposition back to where we started. */ - if (cur != (off_t) -1) + if (cur != (off_t) -1) { + int save_errno = errno; ignore_value(lseek(fd, cur, SEEK_SET)); + errno = save_errno; + } return ret; } -- 2.13.0
On 06/05/2017 04:23 AM, Michal Privoznik wrote:
Callers might be interested in the original value of errno. Let's not overwrite it with lseek() done in cleanup path.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfile.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
One could argue that if the original lseek fails, rather than the goto cleanup, the return -1 could be done. That way, the "if" condition in cleanup: isn't necessary... Additionally, if the second if the second lseek fails to reposition back to where we started and we don't tell the user that, then we're off in never never land possibly, right? Of course IIRC the argument about this was that lseek is highly unlikely to fail in that second scenario. Up to this point we don't say/guarantee anything about the errno return value generally, but I see the subsequent patch adds some verbiage - whether it can be accurately held over any libvirt call perhaps remains to be seen. Still the premise of this is, don't mess with an errno that was set to something with a second one that could change the original intention. Before the ACK/R-B applies, is there any odd special casing that needs to be done for that ENXIO case? That is setting errno = 0 if we decide to not goto cleanup as I don't think in that case we're considering an errno value to be problematic, instead it'd be expected. As long as the errno != ENXIO case is handled... Since we learned nothing and are moving on happily. Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/util/virfile.c b/src/util/virfile.c index d444b32f8..2be64f1db 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.cf @@ -3900,8 +3900,11 @@ virFileInData(int fd, ret = 0; cleanup: /* At any rate, reposition back to where we started. */ - if (cur != (off_t) -1) + if (cur != (off_t) -1) { + int save_errno = errno; ignore_value(lseek(fd, cur, SEEK_SET)); + errno = save_errno; + } return ret; }
All of these four functions (virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll) take one or more callback that handle various aspects of streams. However, if any 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@redhat.com> --- include/libvirt/libvirt-stream.h | 17 +++++++++++++- src/libvirt-stream.c | 50 +++++++++++++++++++++++++++++++++------- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/include/libvirt/libvirt-stream.h b/include/libvirt/libvirt-stream.h index d18d43140..86f96b158 100644 --- a/include/libvirt/libvirt-stream.h +++ b/include/libvirt/libvirt-stream.h @@ -82,7 +82,10 @@ int virStreamRecvHole(virStreamPtr, * of bytes. The callback will continue to be * invoked until it indicates the end of the source * has been reached by returning 0. A return value - * of -1 at any time will abort the send operation + * of -1 at any time will abort the send operation. + * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. * * Returns the number of bytes filled, 0 upon end * of file, or -1 upon error @@ -119,6 +122,9 @@ int virStreamSendAll(virStreamPtr st, * This function should not adjust the current position within * the file. * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns 0 on success, * -1 upon error */ @@ -142,6 +148,9 @@ typedef int (*virStreamSourceHoleFunc)(virStreamPtr st, * processing the hole in the stream source and then return. * A return value of -1 at any time will abort the send operation. * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns 0 on success, * -1 upon error. */ @@ -176,6 +185,9 @@ int virStreamSparseSendAll(virStreamPtr st, * has been reached. A return value of -1 at any time * will abort the receive operation * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns the number of bytes consumed or -1 upon * error */ @@ -203,6 +215,9 @@ int virStreamRecvAll(virStreamPtr st, * hole in the stream target and then return. A return value of * -1 at any time will abort the receive operation. * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns 0 on success, * -1 upon error */ diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index 1594ed212..cf1b2293a 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -567,7 +567,7 @@ virStreamInData(virStreamPtr stream, * * Returns -1 upon any error, with virStreamAbort() already * having been called, so the caller need only call - * virStreamFree() + * virStreamFree(). */ int virStreamSendAll(virStreamPtr stream, @@ -595,9 +595,15 @@ virStreamSendAll(virStreamPtr stream, for (;;) { int got, offset = 0; + + errno = 0; got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("send handler failed")); goto cleanup; + } if (got == 0) break; while (offset < got) { @@ -732,17 +738,24 @@ int virStreamSparseSendAll(virStreamPtr stream, size_t want = bufLen; const unsigned int skipFlags = 0; + errno = 0; if (!dataLen) { - if (holeHandler(stream, &inData, §ionLen, opaque) < 0) + if (holeHandler(stream, &inData, §ionLen, opaque) < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("send holeHandler failed")); goto cleanup; + } if (!inData && sectionLen) { if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) goto cleanup; if (skipHandler(stream, sectionLen, opaque) < 0) { + if (errno == 0) + errno = EIO; virReportSystemError(errno, "%s", - _("unable to skip hole")); + _("send skipHandler failed")); goto cleanup; } continue; @@ -755,8 +768,13 @@ int virStreamSparseSendAll(virStreamPtr stream, want = dataLen; got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", + _("send handler failed")); goto cleanup; + } if (got == 0) break; while (offset < got) { @@ -854,6 +872,8 @@ virStreamRecvAll(virStreamPtr stream, for (;;) { int got, offset = 0; + + errno = 0; got = virStreamRecv(stream, bytes, want); if (got < 0) goto cleanup; @@ -862,8 +882,13 @@ virStreamRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) + if (done < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", + _("recv handler failed")); goto cleanup; + } offset += done; } } @@ -971,13 +996,18 @@ virStreamSparseRecvAll(virStreamPtr stream, long long holeLen; const unsigned int holeFlags = 0; + errno = 0; got = virStreamRecvFlags(stream, bytes, want, flags); if (got == -3) { if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) goto cleanup; - if (holeHandler(stream, holeLen, opaque) < 0) + if (holeHandler(stream, holeLen, opaque) < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("recv holeHandler failed")); goto cleanup; + } continue; } else if (got < 0) { goto cleanup; @@ -987,8 +1017,12 @@ virStreamSparseRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) + if (done < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("recv handler failed")); goto cleanup; + } offset += done; } } -- 2.13.0
On 06/05/2017 04:23 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
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
s/should//
error is not properly reported.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-stream.h | 17 +++++++++++++- src/libvirt-stream.c | 50 +++++++++++++++++++++++++++++++++------- 2 files changed, 58 insertions(+), 9 deletions(-)
diff --git a/include/libvirt/libvirt-stream.h b/include/libvirt/libvirt-stream.h index d18d43140..86f96b158 100644 --- a/include/libvirt/libvirt-stream.h +++ b/include/libvirt/libvirt-stream.h @@ -82,7 +82,10 @@ int virStreamRecvHole(virStreamPtr, * of bytes. The callback will continue to be * invoked until it indicates the end of the source * has been reached by returning 0. A return value - * of -1 at any time will abort the send operation + * of -1 at any time will abort the send operation. + * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. * * Returns the number of bytes filled, 0 upon end * of file, or -1 upon error @@ -119,6 +122,9 @@ int virStreamSendAll(virStreamPtr st, * This function should not adjust the current position within * the file. * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns 0 on success, * -1 upon error */ @@ -142,6 +148,9 @@ typedef int (*virStreamSourceHoleFunc)(virStreamPtr st, * processing the hole in the stream source and then return. * A return value of -1 at any time will abort the send operation. * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns 0 on success, * -1 upon error. */ @@ -176,6 +185,9 @@ int virStreamSparseSendAll(virStreamPtr st, * has been reached. A return value of -1 at any time * will abort the receive operation * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns the number of bytes consumed or -1 upon * error */ @@ -203,6 +215,9 @@ int virStreamRecvAll(virStreamPtr st, * hole in the stream target and then return. A return value of * -1 at any time will abort the receive operation. * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns 0 on success, * -1 upon error */ diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index 1594ed212..cf1b2293a 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -567,7 +567,7 @@ virStreamInData(virStreamPtr stream, * * Returns -1 upon any error, with virStreamAbort() already * having been called, so the caller need only call - * virStreamFree() + * virStreamFree().
Noted, spurious, IDC ;-)
*/ int virStreamSendAll(virStreamPtr stream, @@ -595,9 +595,15 @@ virStreamSendAll(virStreamPtr stream,
for (;;) { int got, offset = 0; + + errno = 0; got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("send handler failed"));
Since errno and vir*LastError are not necessarily linked - do we really want to write an error message in the event that a different message was already in the pipeline? That is should all of the new virReportSystemError calls be prefixed by: if (!virGetLastError()) or perhaps use virGetLastErrorMessage ? John
goto cleanup; + } if (got == 0) break; while (offset < got) { @@ -732,17 +738,24 @@ int virStreamSparseSendAll(virStreamPtr stream, size_t want = bufLen; const unsigned int skipFlags = 0;
+ errno = 0; if (!dataLen) { - if (holeHandler(stream, &inData, §ionLen, opaque) < 0) + if (holeHandler(stream, &inData, §ionLen, opaque) < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("send holeHandler failed")); goto cleanup; + }
if (!inData && sectionLen) { if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) goto cleanup;
if (skipHandler(stream, sectionLen, opaque) < 0) { + if (errno == 0) + errno = EIO; virReportSystemError(errno, "%s", - _("unable to skip hole")); + _("send skipHandler failed")); goto cleanup; } continue; @@ -755,8 +768,13 @@ int virStreamSparseSendAll(virStreamPtr stream, want = dataLen;
got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", + _("send handler failed")); goto cleanup; + } if (got == 0) break; while (offset < got) { @@ -854,6 +872,8 @@ virStreamRecvAll(virStreamPtr stream,
for (;;) { int got, offset = 0; + + errno = 0; got = virStreamRecv(stream, bytes, want); if (got < 0) goto cleanup; @@ -862,8 +882,13 @@ virStreamRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) + if (done < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", + _("recv handler failed")); goto cleanup; + } offset += done; } } @@ -971,13 +996,18 @@ virStreamSparseRecvAll(virStreamPtr stream, long long holeLen; const unsigned int holeFlags = 0;
+ errno = 0; got = virStreamRecvFlags(stream, bytes, want, flags); if (got == -3) { if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) goto cleanup;
- if (holeHandler(stream, holeLen, opaque) < 0) + if (holeHandler(stream, holeLen, opaque) < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("recv holeHandler failed")); goto cleanup; + } continue; } else if (got < 0) { goto cleanup; @@ -987,8 +1017,12 @@ virStreamSparseRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) + if (done < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("recv handler failed")); goto cleanup; + } offset += done; } }
On 06/13/2017 01:03 AM, John Ferlan wrote:
On 06/05/2017 04:23 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
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
s/should//
error is not properly reported.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-stream.h | 17 +++++++++++++- src/libvirt-stream.c | 50 +++++++++++++++++++++++++++++++++------- 2 files changed, 58 insertions(+), 9 deletions(-)
@@ -595,9 +595,15 @@ virStreamSendAll(virStreamPtr stream,
for (;;) { int got, offset = 0; + + errno = 0; got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("send handler failed"));
Since errno and vir*LastError are not necessarily linked - do we really want to write an error message in the event that a different message was already in the pipeline?
That is should all of the new virReportSystemError calls be prefixed by:
if (!virGetLastError())
or perhaps use virGetLastErrorMessage ?
I don't think so. The only path we can get to this virReportSystemError() call is by @handler failing. However, @handler is expected not to set any libvirt error. Therefore there is no message in the pipeline. Michal
On 06/05/2017 10:22 AM, Michal Privoznik wrote:
diff to v3:
-In the last patch, instead of reporting errors from callbacks, report system error based on errno set by the callbacks.
Michal Privoznik (6): virfdstream: Check for thread error more frequently fdstream: Report error from the I/O thread virStream*All: Call virStreamAbort() more frequently virStream*All: Preserve reported error virFileInData: preserve errno in cleanup path virStream*All: Report error if a callback fails
daemon/stream.c | 18 ++++++--- include/libvirt/libvirt-stream.h | 17 +++++++- src/libvirt-stream.c | 83 +++++++++++++++++++++++++++++++--------- src/util/virfdstream.c | 22 ++++++++--- src/util/virfile.c | 5 ++- 5 files changed, 113 insertions(+), 32 deletions(-)
Ping? I'd like to backport these and couple of other stream fixes that I've pushed earlier onto a stable branch since current release has broken streams. Michal
participants (2)
-
John Ferlan -
Michal Privoznik