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

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&gt; > --- > 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?

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