On Sat, May 26, 2018 at 08:27:47AM -0400, John Ferlan wrote:
In a previous commit:
commit d4bf8f415074759baf051644559e04fe78888f8b
Author: Daniel P. Berrangé <berrange(a)redhat.com>
Date: Wed Feb 14 09:43:59 2018 +0000
nwfilter: handle missing switch enum cases
Ensure all enum cases are listed in switch statements, or cast away
enum type in places where we don't wish to cover all cases.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
we changed a switch in the nwfilter learning thread so that it had
explict cases for all enum entries. Unfortunately the parameters in the
method had been declared with incorrect type. The "howDetect" parameter
does *not* accept "enum howDetect" values, rather it accepts a bitmask
of "enum howDetect" values, so it should have been an "int" type.
The caller always passes DETECT_STATIC|DETECT_DHCP, so essentially the
IP addressing learning was completely broken by the above change, as it
never matched any switch case, hitting the default leading to EINVAL.
Rather than treat the howDetect as a numerical enum, let's treat it
as a bitmask/flag since that's the way it's used. Thus, the switch
changes to a simple if bit is set keeping the original intention that
if @howDetect == DETECT_DHCP, then use applyDHCPOnlyRules; otherwise,
use applyBasicRules to determine the IP Address.
Proposed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
Here's to hoping I've used the correct magic incantation to get this
to reply to Daniel's poll series as this should be pushed in
conjunction with that (and thus would need review). This should be
done before the 4.4.0 release.
BTW: This also fixes a strange phenomena I was seeing in my testing
environment where domain startups would periodically fail with:
error: Failed to start domain f23
error: Machine 'qemu-6-f23' already exists
but could be started with enough retry attempts. What causes me
to believe there's a relationship with this series is that if
running libvirtd debug, I see the following as output:
Detaching after fork from child process 22442.
2018-05-25 20:37:24.966+0000: 25923: error :
virSystemdCreateMachine:361 : Machine 'qemu-6-f23' already exists
2018-05-25 20:37:24.988+0000: 22440: error :
learnIPAddressThread:668 : encountered an error on interface
vnet0 index 150: Invalid argument
[Thread 0x7fffacdb7700 (LWP 22440) exited]
With these two patches in place, no more errors.
src/nwfilter/nwfilter_learnipaddr.c | 27 +++++++++++++--------------
src/nwfilter/nwfilter_learnipaddr.h | 10 +++++-----
2 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
index 506b98fd07..ab2697274c 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -145,7 +145,7 @@ struct _virNWFilterIPAddrLearnReq {
char *filtername;
virHashTablePtr filterparams;
virNWFilterDriverStatePtr driver;
- enum howDetect howDetect;
+ virNWFilterLearnIPHowDetectFlags howDetect;
We don't normally use typedefs for bitmasks because they are misleading
as evidenced by this bug. So I still prefer my original patch that uses
'int'.
We could just simplify the code though. Given that it is hardcoded to
always pass DETECT_DHCP | DETECT_STATIC, there's little obvious point
in it being configurable right now, so we could just remove the enum
entirely and hardwire the only thing we actually use.
Regards,
Daniel
--
|:
https://berrange.com -o-
https://www.flickr.com/photos/dberrange :|
|:
https://libvirt.org -o-
https://fstop138.berrange.com :|
|:
https://entangle-photo.org -o-
https://www.instagram.com/dberrange :|