Re: [libvirt] [PATCH v3 1/6] virfdstream: Check for thread error more frequently

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(a)redhat.com&gt; >>>> --- >>>> 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