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@xxxxxxxxxxxxx>
> ---
> 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();
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
