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

[John Ferlan]

On 06/01/2018 06:33 AM, Daniel P. Berrangé wrote:
> 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'.
> 

Yay - more unwritten rules ;-)

OK, sure usually I see bitmasks and unsigned int flags in use...  Still
the value is used as a bitmask in one instance and a constant in
another. Fortunately (1 << 0) and (1 << 1) worked out to the same thing
as the existing code. In the long run, it doesn't really matter to me
how that's "worked out". Using = 1 and = 2, then later using as a
bitmask is probably wrong - it's a consistency thing.

As an aside, passing a typedef enum as a parameter is done a few times
such as virDomainPCIConnectFlags, qemuBlockIoTuneSetFlags, and
virFileCloseFlags using a quick search.

> 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.
> 

Yeah - I thought of that too, but fear of any unknown future plans and
history of the code kept me away from taking that option.

I suppose taking the hardwire route or perhaps going with unsigned int
howDetect; would be fine... I also think that instead of a switch the if
then else would be fine.

John