Re: [libvirt] [PATCH] nwfilter: fix learning address thread shutdown
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(a)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(a)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();
>>>