[PATCH 3/3] logging: fix endless loop on EOF

[Nikolay Shirokovskiy]

On 01.10.2020 17:38, Michal Prívozník wrote:
> On 9/24/20 3:24 PM, Nikolay Shirokovskiy wrote:
>> On EOF condition we always have POLLHUP event and read returns
>> 0 thus we never break loop in virLogHandlerDomainLogFileDrain.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>> ---
>>   src/logging/log_handler.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c
>> index c04f341..152ca24 100644
>> --- a/src/logging/log_handler.c
>> +++ b/src/logging/log_handler.c
>> @@ -464,6 +464,8 @@ virLogHandlerDomainLogFileDrain(virLogHandlerLogFilePtr file)
>>               if (errno == EINTR)
>>                   continue;
>>               return;
>> +        } else if (len == 0) {
> 
> Shouldn't we move "file->drained = true;" from above (not seen in the context though) into this branch and possibly the "if (len < 0)" branch? I mean, if we haven't read all the data the pipe is not drained, is it?

I guess current code is ok. May be we should name this flag another way. It is used to handle race with event loop
thread polling pipe too [1]:

    The solution is to ensure we have drained all pending data from the pipe
    fd before reporting the log file offset. The pipe fd is always in
    blocking mode, so cares needs to be taken to avoid blocking. When
    draining this is taken care of by using poll(). The extra complication
    is that they might already be an event loop dispatch pending on the pipe
    fd. If we have just drained the pipe this pending event will be invalid
    so must be discarded.

So if we saw read poll event on pipe we need to set this flag. May we should name
it discardEvent or something.

[1] 
commit cc9e80c59368478d179ee3eb7bf8106664c56870
Author: Daniel P. Berrangé <berrange at redhat.com>
Date:   Fri Dec 14 12:57:37 2018 +0000

    logging: ensure pending I/O is drained before reading position

Nikolay