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@xxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
Date: Fri Dec 14 12:57:37 2018 +0000
logging: ensure pending I/O is drained before reading position
Nikolay
