[libvirt] [PATCH] nwfilter: fix IP address learning

[John Ferlan]

On 05/18/2018 07:59 AM, Daniel P. Berrangé 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.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/nwfilter/nwfilter_learnipaddr.c | 13 ++++++-------
>  src/nwfilter/nwfilter_learnipaddr.h |  2 +-
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 

This is a companion patch to:

https://www.redhat.com/archives/libvir-list/2018-May/msg01484.html

Still, perhaps to avoid future issues the howDetect related code should
adopt a bitmask model...

Since  Daniel is away and it'd be good to get this into this release
(before he returns) I'll propose an alternative...

John


> diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
> index cc3bfd971c..061b39d72b 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.c
> +++ b/src/nwfilter/nwfilter_learnipaddr.c
> @@ -144,7 +144,7 @@ struct _virNWFilterIPAddrLearnReq {
>      char *filtername;
>      virHashTablePtr filterparams;
>      virNWFilterDriverStatePtr driver;
> -    enum howDetect howDetect;
> +    int howDetect; /* bitmask of enum howDetect */
>  
>      int status;
>      volatile bool terminate;
> @@ -442,23 +442,22 @@ learnIPAddressThread(void *arg)
>          if (techdriver->applyDHCPOnlyRules(req->ifname,
>                                             &req->macaddr,
>                                             NULL, false) < 0) {
> +            VIR_DEBUG("Unable to apply DHCP only rules");
>              req->status = EINVAL;
>              goto done;
>          }
>          virBufferAddLit(&buf, "src port 67 and dst port 68");
>          break;
> -    case DETECT_STATIC:
> +    default:
>          if (techdriver->applyBasicRules(req->ifname,
>                                          &req->macaddr) < 0) {
> +            VIR_DEBUG("Unable to apply basic rules");
>              req->status = EINVAL;
>              goto done;
>          }
>          virBufferAsprintf(&buf, "ether host %s or ether dst ff:ff:ff:ff:ff:ff",
>                            macaddr);
>          break;
> -    default:
> -        req->status = EINVAL;
> -        goto done;
>      }
>  
>      if (virBufferError(&buf)) {
> @@ -693,7 +692,7 @@ learnIPAddressThread(void *arg)
>   *               once its IP address has been detected
>   * @driver : the network filter driver
>   * @howDetect : the method on how the thread is supposed to detect the
> - *              IP address; must choose any of the available flags
> + *              IP address; bitmask of "enum howDetect" flags.
>   *
>   * Instruct to learn the IP address being used on a given interface (ifname).
>   * Unless there already is a thread attempting to learn the IP address
> @@ -711,7 +710,7 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver,
>                            const char *filtername,
>                            virHashTablePtr filterparams,
>                            virNWFilterDriverStatePtr driver,
> -                          enum howDetect howDetect)
> +                          int howDetect)
>  {
>      int rc;
>      virThread thread;
> diff --git a/src/nwfilter/nwfilter_learnipaddr.h b/src/nwfilter/nwfilter_learnipaddr.h
> index 06fea5bff8..753aabc594 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.h
> +++ b/src/nwfilter/nwfilter_learnipaddr.h
> @@ -43,7 +43,7 @@ int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver,
>                                const char *filtername,
>                                virHashTablePtr filterparams,
>                                virNWFilterDriverStatePtr driver,
> -                              enum howDetect howDetect);
> +                              int howDetect);
>  
>  bool virNWFilterHasLearnReq(int ifindex);
>  int virNWFilterTerminateLearnReq(const char *ifname);
>