On Sat, May 26, 2018 at 08:27:47AM -0400, John Ferlan wrote:
> In a previous commit:
>
> commit d4bf8f415074759baf051644559e04fe78888f8b
> Author: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
>
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
