[libvirt] [PATCH 2/1] nwfilter: Fix IP address learning

[Daniel P. Berrangé]

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