[libvirt] [PATCH V11 3/7] nwfilter: Fix support for trusted DHCP servers

[Daniel Veillard]

On Tue, Apr 17, 2012 at 10:44:04AM -0400, Stefan Berger wrote:
> Fix the support for trusted DHCP server in the ebtables code's
> hard-coded function applying DHCP only filtering rules:
> Rather than using a char * use the more flexible
> virNWFilterVarValuePtr that contains the trusted DHCP server(s)
> IP address. Process all entries.
> 
> Since all callers so far provided NULL as parameter, no changes
> are necessary in any other code.
> 
> ---
>  src/conf/nwfilter_conf.h                  |    2 
>  src/nwfilter/nwfilter_ebiptables_driver.c |   72 +++++++++++++++++-------------
>  2 files changed, 44 insertions(+), 30 deletions(-)
> 
> Index: libvirt-acl/src/conf/nwfilter_conf.h
> ===================================================================
> --- libvirt-acl.orig/src/conf/nwfilter_conf.h
> +++ libvirt-acl/src/conf/nwfilter_conf.h
> @@ -625,7 +625,7 @@ typedef int (*virNWFilterApplyBasicRules
>  
>  typedef int (*virNWFilterApplyDHCPOnlyRules)(const char *ifname,
>                                               const unsigned char *macaddr,
> -                                             const char *dhcpserver,
> +                                             virNWFilterVarValuePtr dhcpsrvs,
>                                               bool leaveTemporary);
>  
>  typedef int (*virNWFilterRemoveBasicRules)(const char *ifname);
> Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
> ===================================================================
> --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c
> +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
> @@ -3195,7 +3195,7 @@ tear_down_tmpebchains:
>   * @ifname: name of the backend-interface to which to apply the rules
>   * @macaddr: MAC address the VM is using in packets sent through the
>   *    interface
> - * @dhcpserver: The DHCP server from which the VM may receive traffic
> + * @dhcpsrvrs: The DHCP server(s) from which the VM may receive traffic
>   *    from; may be NULL
>   * @leaveTemporary: Whether to leave the table names with their temporary
>   *    names (true) or also perform the renaming to their final names as
> @@ -3209,14 +3209,15 @@ tear_down_tmpebchains:
>  static int
>  ebtablesApplyDHCPOnlyRules(const char *ifname,
>                             const unsigned char *macaddr,
> -                           const char *dhcpserver,
> +                           virNWFilterVarValuePtr dhcpsrvrs,
>                             bool leaveTemporary)
>  {
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      char chain_in [MAX_CHAINNAME_LENGTH],
>           chain_out[MAX_CHAINNAME_LENGTH];
>      char macaddr_str[VIR_MAC_STRING_BUFLEN];
> -    char *srcIPParam = NULL;
> +    unsigned int idx = 0;
> +    unsigned int num_dhcpsrvrs;
>  
>      if (!ebtables_cmd_path) {
>          virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -3225,15 +3226,6 @@ ebtablesApplyDHCPOnlyRules(const char *i
>          return -1;
>      }
>  
> -    if (dhcpserver) {
> -        virBufferAsprintf(&buf, " --ip-src %s", dhcpserver);
> -        if (virBufferError(&buf)) {
> -            virBufferFreeAndReset(&buf);
> -            return -1;
> -        }
> -        srcIPParam = virBufferContentAndReset(&buf);
> -    }
> -
>      virMacAddrFormat(macaddr, macaddr_str);
>  
>      ebiptablesAllTeardown(ifname);
> @@ -3267,20 +3259,46 @@ ebtablesApplyDHCPOnlyRules(const char *i
>                        chain_in,
>                        CMD_STOPONERR(1));
>  
> -    virBufferAsprintf(&buf,
> -                      CMD_DEF("$EBT -t nat -A %s"
> -                              " -d %s"
> -                              " -p ipv4 --ip-protocol udp"
> -                              " %s"
> -                              " --ip-sport 67 --ip-dport 68"
> -                              " -j ACCEPT") CMD_SEPARATOR
> -                      CMD_EXEC
> -                      "%s",
> +    num_dhcpsrvrs = (dhcpsrvrs != NULL)
> +                    ? virNWFilterVarValueGetCardinality(dhcpsrvrs)
> +                    : 0;
>  
> -                      chain_out,
> -                      macaddr_str,
> -                      srcIPParam != NULL ? srcIPParam : "",
> -                      CMD_STOPONERR(1));
> +    while (true) {
> +        char *srcIPParam = NULL;
> +
> +        if (idx < num_dhcpsrvrs) {
> +            const char *dhcpserver;
> +
> +            dhcpserver = virNWFilterVarValueGetNthValue(dhcpsrvrs, idx);
> +
> +            if (virAsprintf(&srcIPParam, "--ip-src %s", dhcpserver) < 0) {
> +                virReportOOMError();
> +                goto tear_down_tmpebchains;
> +            }
> +        }
> +
> +        virBufferAsprintf(&buf,
> +                          CMD_DEF("$EBT -t nat -A %s"
> +                                  " -d %s"
> +                                  " -p ipv4 --ip-protocol udp"
> +                                  " %s"
> +                                  " --ip-sport 67 --ip-dport 68"
> +                                  " -j ACCEPT") CMD_SEPARATOR
> +                          CMD_EXEC
> +                          "%s",
> +
> +                          chain_out,
> +                          macaddr_str,
> +                          srcIPParam != NULL ? srcIPParam : "",
> +                          CMD_STOPONERR(1));
> +
> +        VIR_FREE(srcIPParam);
> +
> +        if (idx == num_dhcpsrvrs)
> +            break;
> +
> +        idx++;
> +    }

  There is something I don't understand in that loop, you repetedly
write to buf, but you don't seems to use buf in the loop. This looks
fishy to me, or are you using side effect execution in the Asprintf
argument evaluation. Too cryptic to my taste, I'm lost !

>      virBufferAsprintf(&buf,
>                        CMD_DEF("$EBT -t nat -A %s -j DROP") CMD_SEPARATOR
> @@ -3301,8 +3319,6 @@ ebtablesApplyDHCPOnlyRules(const char *i
>      if (ebiptablesExecCLI(&buf, NULL, NULL) < 0)
>          goto tear_down_tmpebchains;
>  
> -    VIR_FREE(srcIPParam);
> -
>      return 0;
>  
>  tear_down_tmpebchains:
> @@ -3312,8 +3328,6 @@ tear_down_tmpebchains:
>                             "%s",
>                             _("Some rules could not be created."));
>  
> -    VIR_FREE(srcIPParam);
> -
>      return -1;
>  }

  If you can explain the rationale for that buf use in the loop, ACK

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/