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

[Nikolay Shirokovskiy]

On 18.10.2018 03:26, John Ferlan wrote:
> 
> 
> 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)
> 

Thanx!

Nikolay

> 
>>>
>>> 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();
>>>>