[libvirt] [PATCH] nwfilter: fix learning address thread shutdown

[John Ferlan]

On 10/17/18 4:25 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 17.10.2018 01:28, John Ferlan wrote:
>>
>>
>> On 10/12/18 3:23 AM, Nikolay Shirokovskiy wrote:
>>> If learning thread is configured to learn on all ethernet frames (which is
>>> hardcoded) then chances are big that there is packet on every iteration of
>>> inspecting frames loop. As result we will hang on shutdown because we don't
>>> check threadsTerminate if there is packet.
>>>
>>> Let's just check termination conditions on every iteration.
>>>
>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>>> ---
>>>  src/nwfilter/nwfilter_learnipaddr.c | 22 +++++++---------------
>>>  1 file changed, 7 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
>>> index 008c24b..e6cb996 100644
>>> --- a/src/nwfilter/nwfilter_learnipaddr.c
>>> +++ b/src/nwfilter/nwfilter_learnipaddr.c
>>> @@ -483,6 +483,12 @@ learnIPAddressThread(void *arg)
>>>      while (req->status == 0 && vmaddr == 0) {
>>>          int n = poll(fds, ARRAY_CARDINALITY(fds), PKT_TIMEOUT_MS);
>>>  
>>> +        if (threadsTerminate || req->terminate) {
>>> +            req->status = ECANCELED;
>>> +            showError = false;
>>> +            break;
>>> +        }
>>> +
>>
>> So the theory is that regardless of whether there is a timeout or not,
>> let's check for termination markers before checking poll call return
>> status.  Which is fine; however, ...
>>
>>>          if (n < 0) {
>>>              if (errno == EAGAIN || errno == EINTR)
>>>                  continue;
>>> @@ -492,15 +498,8 @@ learnIPAddressThread(void *arg)
>>>              break;
>>>          }
>>>  
>>> -        if (n == 0) {
>>> -            if (threadsTerminate || req->terminate) {
>>> -                VIR_DEBUG("Terminate request seen, cancelling pcap");
>>> -                req->status = ECANCELED;
>>> -                showError = false;
>>> -                break;
>>> -            }
>>> +        if (n == 0)
>>>              continue;
>>> -        }
>>>  
>>>          if (fds[0].revents & (POLLHUP | POLLERR)) {
>>>              VIR_DEBUG("Error from FD probably dev deleted");
>>> @@ -512,13 +511,6 @@ learnIPAddressThread(void *arg)
>>>          packet = pcap_next(handle, &header);
>>>  
>>>          if (!packet) {
>>> -            /* Already handled with poll, but lets be sure */
>>> -            if (threadsTerminate || req->terminate) {
>>> -                req->status = ECANCELED;
>>> -                showError = false;
>>> -                break;
>>> -            }
>>> -
>>
>> Why remove this one? Is it possible termination was set after the poll
>> and if fetching the packet fails, then if these are true let's get out
>> before possibly continuing back to the poll (which I assume would
>> timeout and fail).  Secondary question would be should this be separated
>> from the other part?
> 
> I guess pcap_next does not takes much time (as it does not wait for IO)
> so there is a little chance things change after the above check. And
> even if they are timeout is small and we terminate with little delay
> right after poll.
> 
> As to the second question I'm not sure why separation is useful.
> 
> Nikolay
> 

OK - I left things as is, slightly edited the commit message w/r/t
grammar stuff...

Reviewed-by: John Ferlan <jferlan at redhat.com>
(and pushed)

John

>>
>> Just need to ask - maybe the answer alters the commit message a little
>> bit too.
>>
>> John
>>
>>>              /* Again, already handled above, but lets be sure */
>>>              if (virNetDevValidateConfig(req->binding->portdevname, NULL, req->ifindex) <= 0) {
>>>                  virResetLastError();
>>>