[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv4] Configure native vlan modes on Open vSwitch ports



On 05/23/2013 01:12 PM, james robson wrote:
> This patch adds functionality to allow libvirt to configure the
> 'native-tagged' and 'native-untagged' modes on openvswitch networks.
>
> v2 changes:
> Fix problems reported by Eric Blake
>
> v3 changes:
> Re work patch to address review comments
>
> v4 changes:
> Use enum for native modes

The comments about differences between revisionis of a patch should be
added with "git send-email --annotate" rather than included in the
commit log message, since want what we push in the end to not include
any of those. Normally people add them below the "---" line (see just
below here).

Not to worry though, since I'll be pushing the patch, I'll just edit the
commit log before I push.

>
> ---
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 9284534..ba32185 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3498,6 +3498,13 @@ qemu-kvm -net nic,model=? /dev/null
>          <parameters interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/>
>        </virtualport>
>      </interface>
> +    <interface type='bridge'>
> +      <b>&lt;vlan trunk='yes'&gt;</b>
> +        <b>&lt;tag id='42'/&gt;</b>
> +        <b>&lt;tag id='123' nativeMode='untagged'/&gt;</b>
> +      <b>&lt;/vlan&gt;</b>
> +      ...
> +    &lt;/interface&gt;
>    &lt;devices&gt;
>    ...</pre>
>  
> @@ -3524,6 +3531,15 @@ qemu-kvm -net nic,model=? /dev/null
>        vlan element.
>      </p>
>  
> +    <p>
> +      For network connections using openvswitch it is possible to
> +      configure the 'native-tagged' and 'native-untagged' vlan modes
> +      <span class="since">(Since 1.0.5).</span> This uses the optional


This is now 1.0.7 :-)


> +      <code>nativeMode</code> attribute on the <code>&lt;tag&gt;</code>
> +      element: <code>nativeMode</code> may be set to 'tagged' or
> +      'untagged'. The id atribute of the element sets the native vlan.
> +    </p>
> +
>      <h5><a name="elementLink">Modifying virtual link state</a></h5>
>  <pre>
>    ...
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index a1198ce..29e12d9 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
> @@ -446,6 +446,13 @@
>          &lt;parameters interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/&gt;
>        &lt;/virtualport&gt;
>      &lt;/interface&gt;
> +    &lt;interface type='bridge'&gt;
> +      <b>&lt;vlan trunk='yes'&gt;</b>
> +        <b>&lt;tag id='42'/&gt;</b>
> +        <b>&lt;tag id='123' nativeMode='untagged'/&gt;</b>
> +      <b>&lt;/vlan&gt;</b>
> +      ...
> +    &lt;/interface&gt;
>    &lt;devices&gt;
>    ...</pre>
>  
> @@ -469,6 +476,14 @@
>        be added to the vlan element.
>      </p>
>      <p>
> +      For network connections using openvswitch it is possible to
> +      configure the 'native-tagged' and 'native-untagged' vlan modes
> +      <span class="since">(Since 1.0.6).</span> This uses the optional


1.0.7


> +      <code>nativeMode</code> attribute on the <code>&lt;tag&gt;</code>
> +      element: <code>nativeMode</code> may be set to 'tagged' or
> +      'untagged'. The id atribute of the element sets the native vlan.
> +    </p>
> +    <p>
>        <code>&lt;vlan&gt;</code> elements can also be specified in
>        a <code>&lt;portgroup&gt;</code> element, as well as directly in
>        a domain's <code>&lt;interface&gt;</code> element. In the case
> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng
> index 51ff759..e60f1fc 100644
> --- a/docs/schemas/networkcommon.rng
> +++ b/docs/schemas/networkcommon.rng
> @@ -204,6 +204,14 @@
>                <param name="maxInclusive">4095</param>
>              </data>
>            </attribute>
> +          <optional>
> +            <attribute name="nativeMode">
> +              <choice>
> +                <value>tagged</value>
> +                <value>untagged</value>
> +              </choice>
> +            </attribute>
> +          </optional>
>            <empty/>
>          </element>
>        </oneOrMore>
> diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c
> index 13ba8c6..2b4cd48 100644
> --- a/src/conf/netdev_vlan_conf.c
> +++ b/src/conf/netdev_vlan_conf.c
> @@ -17,6 +17,7 @@
>   *
>   * Authors:
>   *     Laine Stump <laine redhat com>
> + *     James Robson <jrobson websense com>
>   */
>  
>  #include <config.h>
> @@ -27,12 +28,15 @@
>  
>  #define VIR_FROM_THIS VIR_FROM_NONE
>  
> +VIR_ENUM_IMPL(virNativeVlanMode, VIR_NATIVE_VLAN_MODE_LAST, "default", "tagged", "untagged")
> +

Yes, that's the stuff. This lack of enum was what I was hung up on last
iteration, if I remember correctly.


>  int
>  virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr def)
>  {
>      int ret = -1;
>      xmlNodePtr save = ctxt->node;
>      const char *trunk = NULL;
> +    const char *nativeMode = NULL;
>      xmlNodePtr *tagNodes = NULL;
>      int nTags, ii;
>  
> @@ -54,6 +58,8 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de
>          goto error;
>      }
>  
> +    def->nativeMode = 0;
> +    def->nativeTag = 0;
>      for (ii = 0; ii < nTags; ii++) {
>          unsigned long id;
>  
> @@ -68,6 +74,19 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de
>                             _("vlan tag id %lu too large (maximum 4095)"), id);
>              goto error;
>          }
> +        if ((nativeMode = virXPathString("string(./@nativeMode)", ctxt)) != NULL) {
> +            if (def->nativeMode != 0) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("duplicate native vlan setting"));
> +                goto error;
> +            }
> +            if ((def->nativeMode = virNativeVlanModeTypeFromString(nativeMode)) <= 0) {
> +                virReportError(VIR_ERR_XML_ERROR,
> +                               _("Attribute \"nativeMode='%s'\" is invalid"), nativeMode);
> +                goto error;
> +            }
> +            def->nativeTag = id;
> +        }
>          def->tag[ii] = id;
>      }
>  
> @@ -89,6 +108,12 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de
>                                   "is required for more than one vlan tag"), trunk);
>                  goto error;
>              }
> +            if (def->nativeMode != 0) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("invalid configuration in <vlan> - \"trunk='no'\" is "
> +                                 "not allowed with a native vlan id"));
> +                goto error;
> +            }
>              /* allow (but discard) "trunk='no' if there is a single tag */
>              if (STRCASENEQ(trunk, "no")) {
>                  virReportError(VIR_ERR_XML_ERROR,
> @@ -125,7 +150,17 @@ virNetDevVlanFormat(virNetDevVlanPtr def, virBufferPtr buf)
>  
>      virBufferAsprintf(buf, "<vlan%s>\n", def->trunk ? " trunk='yes'" : "");
>      for (ii = 0; ii < def->nTags; ii++) {
> -        virBufferAsprintf(buf, "  <tag id='%u'/>\n", def->tag[ii]);
> +        if (def->nativeMode != VIR_NATIVE_VLAN_MODE_DEFAULT && def->nativeTag == def->tag[ii]) {
> +            /* check the nativeMode in case we get <tag id='0'/>*/
> +            const char *mode = virNativeVlanModeTypeToString(def->nativeMode);
> +            if (!mode) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("Bad vlaue for nativeMode"));
> +            }
> +            virBufferAsprintf(buf, "  <tag id='%u' nativeMode='%s'/>\n", def->tag[ii], mode);
> +        } else {
> +            virBufferAsprintf(buf, "  <tag id='%u'/>\n", def->tag[ii]);
> +        }
>      }
>      virBufferAddLit(buf, "</vlan>\n");
>      return 0;
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index 2aee445..47e6027 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -109,8 +109,22 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>      virCommandAddArgList(cmd, "--timeout=5", "--", "--may-exist", "add-port",
>                          brname, ifname, NULL);
>  
> -    if (virBufferUse(&buf) != 0)
> +    if (virBufferUse(&buf) != 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;
> +        }

I don't see a reason for putting this inside the "if
virBufferUse(&buf)", since that will always be true as long as
virtVlan->nTags is at least 1, and there would be no way
virtVlan->nativMode to be set it nTags was 0.

So I moved it out - if virtVlan->nativeMode == DEFAULT, then no args
will be output.

>          virCommandAddArgList(cmd, virBufferCurrentContent(&buf), NULL);
> +    }
>  
>      if (ovsport->profileID[0] == '\0') {
>          virCommandAddArgList(cmd,
> diff --git a/src/util/virnetdevvlan.c b/src/util/virnetdevvlan.c
> index 2fe2017..eed32f7 100644
> --- a/src/util/virnetdevvlan.c
> +++ b/src/util/virnetdevvlan.c
> @@ -33,6 +33,8 @@ virNetDevVlanClear(virNetDevVlanPtr vlan)
>  {
>      VIR_FREE(vlan->tag);
>      vlan->nTags = 0;
> +    vlan->nativeMode = 0;
> +    vlan->nativeTag = 0;
>  }
>  
>  void
> @@ -54,7 +56,9 @@ virNetDevVlanEqual(const virNetDevVlanPtr a, const virNetDevVlanPtr b)
>          return false;
>  
>      if (a->trunk != b->trunk ||
> -        a->nTags != b->nTags) {
> +        a->nTags != b->nTags ||
> +        a->nativeMode != b->nativeMode ||
> +        a->nativeTag != b->nativeTag) {
>          return false;
>      }
>  
> @@ -89,6 +93,8 @@ virNetDevVlanCopy(virNetDevVlanPtr dst, const virNetDevVlanPtr src)
>  
>      dst->trunk = src->trunk;
>      dst->nTags = src->nTags;
> +    dst->nativeMode = src->nativeMode;
> +    dst->nativeTag = src->nativeTag;
>      memcpy(dst->tag, src->tag, src->nTags * sizeof(*src->tag));
>      return 0;
>  }
> diff --git a/src/util/virnetdevvlan.h b/src/util/virnetdevvlan.h
> index c6b16ef..a084938 100644
> --- a/src/util/virnetdevvlan.h
> +++ b/src/util/virnetdevvlan.h
> @@ -18,16 +18,28 @@
>   * Authors:
>   *      Laine Stump <laine redhat com>
>   */
> -
> +# include <virutil.h>

You should put required #includes *after* the #ifndef __File_Identifier

>  #ifndef __VIR_NETDEV_VLAN_H__
>  # define __VIR_NETDEV_VLAN_H__
>  
> +typedef enum {
> +    VIR_NATIVE_VLAN_MODE_DEFAULT = 0,
> +    VIR_NATIVE_VLAN_MODE_TAGGED,
> +    VIR_NATIVE_VLAN_MODE_UNTAGGED,
> +
> +    VIR_NATIVE_VLAN_MODE_LAST
> +} virNativeVlanMode;
> +
> +VIR_ENUM_DECL(virNativeVlanMode)
> +
>  typedef struct _virNetDevVlan virNetDevVlan;
>  typedef virNetDevVlan *virNetDevVlanPtr;
>  struct _virNetDevVlan {
>      bool trunk;        /* true if this is a trunk */
>      int nTags;          /* number of tags in array */
>      unsigned int *tag; /* pointer to array of tags */
> +    int nativeMode;
> +    unsigned int nativeTag;
>  };
>  
>  void virNetDevVlanClear(virNetDevVlanPtr vlan);
> diff --git a/tests/networkxml2xmlin/openvswitch-net.xml b/tests/networkxml2xmlin/openvswitch-net.xml
> index a3d82b1..2f6084d 100644git gggggdfdffdfdfdfdf
> --- a/tests/networkxml2xmlin/openvswitch-net.xml
> +++ b/tests/networkxml2xmlin/openvswitch-net.xml
> @@ -21,4 +21,13 @@
>        <parameters profileid='alice-profile'/>
>      </virtualport>
>    </portgroup>
> +  <portgroup name='native'>
> +    <vlan trunk='yes'>
> +      <tag id='123' nativeMode='tagged'/>
> +      <tag id='444'/>
> +    </vlan>
> +    <virtualport>
> +      <parameters profileid='native-profile'/>
> +    </virtualport>
> +  </portgroup>
>  </network>
> diff --git a/tests/networkxml2xmlout/openvswitch-net.xml b/tests/networkxml2xmlout/openvswitch-net.xml
> index a3d82b1..2f6084d 100644
> --- a/tests/networkxml2xmlout/openvswitch-net.xml
> +++ b/tests/networkxml2xmlout/openvswitch-net.xml
> @@ -21,4 +21,13 @@
>        <parameters profileid='alice-profile'/>
>      </virtualport>
>    </portgroup>
> +  <portgroup name='native'>
> +    <vlan trunk='yes'>
> +      <tag id='123' nativeMode='tagged'/>
> +      <tag id='444'/>
> +    </vlan>
> +    <virtualport>
> +      <parameters profileid='native-profile'/>
> +    </virtualport>
> +  </portgroup>
>  </network>

ACK. I've made the small changes I indicated above and pushed. Since
we're heading into freeze for v1.0.7 now, it would be a really good idea
for you to pull the latest from git, built it, and make sure it still
works as you intended - there's still about a week to fix any bugs that
may have crept in.

Thanks for your contribution!


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]