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

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&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? > > 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&gt; (and pushed)

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