src/nwfilter/nwfilter_learnipaddr.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)
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@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;
+ }
+
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;
- }
-
/* Again, already handled above, but lets be sure */
if (virNetDevValidateConfig(req->binding->portdevname, NULL, req->ifindex) <= 0) {
virResetLastError();
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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?
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
>
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com>
(and pushed)
John
>>
>> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@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();
>>>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.