[libvirt] [PATCH 1/2] Refactored OVS VLAN configuration

[Michal Privoznik]

On 07/17/2017 05:48 PM, Antoine Millet wrote:
> Moved OVS VLAN configuration arguments construction into a separated
> function to be reused.
> ---
>  src/util/virnetdevopenvswitch.c | 104 ++++++++++++++++++++++++----------------
>  src/util/virnetdevopenvswitch.h |   5 ++
>  2 files changed, 68 insertions(+), 41 deletions(-)
> 
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index a5ecfb691..89ed0c91c 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -64,6 +64,67 @@ virNetDevOpenvswitchAddTimeout(virCommandPtr cmd)
>  }
>  
>  /**
> + * virNetDevOpenvswitchConstructVlans:
> + * @cmd: command to construct
> + * @virtVlan: VLAN configuration to be applied
> + *
> + * Construct the VLAN configuration parameters to be passed to ovs-vsctl command.
> + *
> + * Returns 0 in case of success or -1 in case of failure.
> + */
> +int virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, virNetDevVlanPtr virtVlan)
> +{
> +    int ret = -1;
> +    size_t i = 0;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    if (virtVlan && virtVlan->nTags > 0) {

How about reversing this so that the rest of the function can be
indented one level to the left? I mean:

if (!virtVlan || !virtVlan->nTags)
    return 0;

switch() {
...

> +
> +        switch (virtVlan->nativeMode) {
> +        case VIR_NATIVE_VLAN_MODE_TAGGED:
> +            virCommandAddArg(cmd, "vlan_mode=native-tagged");
> +            virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
> +            break;
> +        case VIR_NATIVE_VLAN_MODE_UNTAGGED:
> +            virCommandAddArg(cmd, "vlan_mode=native-untagged");
> +            virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
> +            break;
> +        case VIR_NATIVE_VLAN_MODE_DEFAULT:
> +        default:
> +            break;
> +        }
> +
> +        if (virtVlan->trunk) {
> +            virBufferAddLit(&buf, "trunk=");
> +
> +            /*
> +             * Trunk ports have at least one VLAN. Do the first one
> +             * outside the "for" loop so we can put a "," at the
> +             * start of the for loop if there are more than one VLANs
> +             * on this trunk port.
> +             */
> +            virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
> +
> +            for (i = 1; i < virtVlan->nTags; i++) {
> +                virBufferAddLit(&buf, ",");
> +                virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
> +            }
> +
> +            if (virBufferCheckError(&buf) < 0)
> +                goto cleanup;
> +            virCommandAddArg(cmd, virBufferCurrentContent(&buf));
> +        } else if (virtVlan->nTags) {
> +            virCommandAddArgFormat(cmd, "tag=%d", virtVlan->tag[0]);
> +        }
> +    }
> +
> +    ret = 0;
> +    cleanup:
> +        virBufferFreeAndReset(&buf);
> +        return ret;

Indent.

> +}
> +
> +/**
>   * virNetDevOpenvswitchAddPort:
>   * @brname: the bridge name
>   * @ifname: the network interface name
> @@ -82,7 +143,6 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>                                     virNetDevVlanPtr virtVlan)
>  {
>      int ret = -1;
> -    size_t i = 0;
>      virCommandPtr cmd = NULL;
>      char macaddrstr[VIR_MAC_STRING_BUFLEN];
>      char ifuuidstr[VIR_UUID_STRING_BUFLEN];
> @@ -91,7 +151,6 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>      char *ifaceid_ex_id = NULL;
>      char *profile_ex_id = NULL;
>      char *vmid_ex_id = NULL;
> -    virBuffer buf = VIR_BUFFER_INITIALIZER;
>  
>      virMacAddrFormat(macaddr, macaddrstr);
>      virUUIDFormat(ovsport->interfaceID, ifuuidstr);
> @@ -117,44 +176,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>      virCommandAddArgList(cmd, "--", "--if-exists", "del-port",
>                           ifname, "--", "add-port", brname, ifname, NULL);
>  
> -    if (virtVlan && virtVlan->nTags > 0) {
> -
> -        switch (virtVlan->nativeMode) {
> -        case VIR_NATIVE_VLAN_MODE_TAGGED:
> -            virCommandAddArg(cmd, "vlan_mode=native-tagged");
> -            virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
> -            break;
> -        case VIR_NATIVE_VLAN_MODE_UNTAGGED:
> -            virCommandAddArg(cmd, "vlan_mode=native-untagged");
> -            virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
> -            break;
> -        case VIR_NATIVE_VLAN_MODE_DEFAULT:
> -        default:
> -            break;
> -        }
> -
> -        if (virtVlan->trunk) {
> -            virBufferAddLit(&buf, "trunk=");
> -
> -            /*
> -             * Trunk ports have at least one VLAN. Do the first one
> -             * outside the "for" loop so we can put a "," at the
> -             * start of the for loop if there are more than one VLANs
> -             * on this trunk port.
> -             */
> -            virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
> -
> -            for (i = 1; i < virtVlan->nTags; i++) {
> -                virBufferAddLit(&buf, ",");
> -                virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
> -            }
> -
> -            if (virBufferCheckError(&buf) < 0)
> -                goto cleanup;
> -            virCommandAddArg(cmd, virBufferCurrentContent(&buf));
> -        } else if (virtVlan->nTags) {
> -            virCommandAddArgFormat(cmd, "tag=%d", virtVlan->tag[0]);
> -        }
> +    if (virNetDevOpenvswitchConstructVlans(cmd, virtVlan) < 0) {
> +        goto cleanup;
>      }

We don't use curly braces in this case.

>  
>      if (ovsport->profileID[0] == '\0') {
> @@ -185,7 +208,6 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>  
>      ret = 0;
>   cleanup:
> -    virBufferFreeAndReset(&buf);
>      VIR_FREE(attachedmac_ex_id);
>      VIR_FREE(ifaceid_ex_id);
>      VIR_FREE(vmid_ex_id);
> diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
> index 51bb1dd00..94b4c695b 100644
> --- a/src/util/virnetdevopenvswitch.h
> +++ b/src/util/virnetdevopenvswitch.h
> @@ -30,11 +30,16 @@
>  # include "internal.h"
>  # include "virnetdevvportprofile.h"
>  # include "virnetdevvlan.h"
> +# include "vircommand.h"
> +
>  
>  # define VIR_NETDEV_OVS_DEFAULT_TIMEOUT 5
>  
>  void virNetDevOpenvswitchSetTimeout(unsigned int timeout);
>  
> +int virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, virNetDevVlanPtr virtVlan)
> +    ATTRIBUTE_RETURN_CHECK;
> +

I don't think you need to export this function. It can be static. Then
you also don't need to include vircommand.h. Also, if you really were to
export the function you'd need to adjust libvirt_private.syms accordingly.

>  int virNetDevOpenvswitchAddPort(const char *brname,
>                                  const char *ifname,
>                                  const virMacAddr *macaddr,
> 

Michal