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