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@xxxxxxxxxx>
>>>> ---
>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
