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