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

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



On May 10, 2013, at 1:10 PM, Laine Stump <laine laine org> wrote:
> On 04/30/2013 02: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
>> 
>> ---
>> 
>> 
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 888c005..5278534 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -3290,6 +3290,13 @@ qemu-kvm -net nic,model=? /dev/null
>>         &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>
>> 
>> @@ -3316,6 +3323,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.6).</span> This uses the optional
>> +      <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 4dd0415..e065ca4 100644
>> --- a/docs/formatnetwork.html.in
>> +++ b/docs/formatnetwork.html.in
>> @@ -414,6 +414,7 @@
>>         <span class="since">Since 0.9.4</span>
>>       </p>
>> 
>> +
>>     <h5><a name="elementVlanTag">Setting VLAN tag (on supported network
>> types only)</a></h5>
>> 
>> <pre>
>> @@ -429,6 +430,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>
>> 
>> @@ -452,6 +460,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
>> +      <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..dd5b64e 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>
>> @@ -33,6 +34,7 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr
>> ctxt, virNetDevVlanPtr de
>>     int ret = -1;
>>     xmlNodePtr save = ctxt->node;
>>     const char *trunk = NULL;
>> +    const char *nativeMode = NULL;
>>     xmlNodePtr *tagNodes = NULL;
>>     int nTags, ii;
>> 
>> @@ -54,6 +56,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 +72,21 @@ 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 (STRCASENEQ(nativeMode, "tagged") &&
>> STRCASENEQ(nativeMode, "untagged")) {
>> +                virReportError(VIR_ERR_XML_ERROR,
>> +                               _("invalid \"nativeMode='%s'\" in <tag>
>> - "
>> +                                 "native_mode must be 'tagged' or
>> 'untagged'"), nativeMode);
>> +                goto error;
>> +            }
>> +            if (def->nativeMode != 0) {
>> +                virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                               _("duplicate native vlan setting"));
>> +                goto error;
>> +            }
>> +            def->nativeMode = STRCASEEQ(nativeMode, "tagged") ? 1 : 2;
> 
> We prefer for things like this to have an enum defined, and use the
> ${enum-type-name}Type(To|From)String to convert between the string and
> the numeric value. Then in the code that uses the value, we compare to
> the enum rather than a plain number. For example, you can search for
> virDomainHostdevSubsys in src/conf/domain_conf.[ch]. Basically you need:
> 
> * in the .h file:
> 
>  typedef enum {
>      VIR_WHATEVER_NAME_TYPE_DEFAULT = 0,
>      VIR_WHATEVER_NAME_TYPE_ONE,
>      VIR_WHATEVER_NAME_TYPE_ANOTHER,
> 
>      VIR_WHATEVER_NAME_TYPE_LAST
>  } virWhateverNameType;
> 
> VIR_ENUM_DECL(virWhateverName)
> 
> * in the .c file:
> 
> VIR_ENUM_IMPL(virWhateverName, VIR_WHATEVER_NAME_TYPE_LAST,
>              "default", "one", "another")
> 
> * at the place you parse:
> 
> if ((def->whatever = virWhateverTypeFromString(type)) <= 0) {
>     virReportError(.....);
>     goto error/cleanup;
> }
> 
> * at the place you format:
> 
> if (def->whatever) {
>    const char *whateverStr = virWhateverTypeToString(def->whatever);
> 
>    if (!whatever) {
>       virReportError(VIR_ERR_INTERNAL_ERROR, "Bad whatever value"...);
>       goto error/cleanup;
>    }
>    virAsprintf(buf, " whatever='%s', whateverStr);
> 
> * in the code that implements it:
> 
> switch (def->whatever) {
> case VIR_WHATEVER_NAME_TYPE_ONE:
>    blah;
>    break;
> case VIR_WHATEVER_NAME_TYPE_ANOTHER:
>    bleh;
>    break;
> case VIR_WHATEVER_NAME_TYPE_DEFAULT:
> default:
>    break;
> }
> 
> or something like that.
> 
> 
>> +            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,11 @@ 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->nativeTag == def->tag[ii]) {
>> +            virBufferAsprintf(buf, "  <tag id='%u' nativeMode='%
>> s'/>\n", def->tag[ii], def->nativeMode == 1 ? "tagged" : "untagged");
>> +        } 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 4fe077a..69cc1ac 100644
>> --- a/src/util/virnetdevopenvswitch.c
>> +++ b/src/util/virnetdevopenvswitch.c
>> @@ -19,6 +19,7 @@
>>  *     Dan Wendlandt <dan nicira com>
>>  *     Kyle Mestery <kmestery cisco com>
>>  *     Ansis Atteka <aatteka nicira com>
>> + *     James Robson <jrobson websense com>
> 
> The author list in our files usually only contains the original authors
> of the file and those who have contributed a "substantial" portion after
> creation.
> 
>>  */
>> 
>> #include <config.h>
>> @@ -108,8 +109,13 @@ 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) {
>> +        if (virtVlan->nativeMode > 0) {
>> +            virCommandAddArgFormat(cmd, "vlan_mode=%s",
>> virtVlan->nativeMode == 1 ? "native-tagged" : "native-untagged");
>> +            virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
>> +        }
>>         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..b81c037 100644
>> --- a/src/util/virnetdevvlan.c
>> +++ b/src/util/virnetdevvlan.c
>> @@ -17,6 +17,7 @@
>>  *
>>  * Authors:
>>  *      Laine Stump <laine redhat com>
>> + *      James Robson <jrobson websense com>
>>  */
>> 
>> #include <config.h>
>> @@ -33,6 +34,8 @@ virNetDevVlanClear(virNetDevVlanPtr vlan)
>> {
>>     VIR_FREE(vlan->tag);
>>     vlan->nTags = 0;
>> +    vlan->nativeMode = 0;
>> +    vlan->nativeTag = 0;
>> }
>> 
>> void
>> @@ -54,7 +57,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 +94,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..93426cc 100644
>> --- a/src/util/virnetdevvlan.h
>> +++ b/src/util/virnetdevvlan.h
>> @@ -17,6 +17,7 @@
>>  *
>>  * Authors:
>>  *      Laine Stump <laine redhat com>
>> + *      James Robson <jrobson websense com>
>>  */
>> 
>> #ifndef __VIR_NETDEV_VLAN_H__
>> @@ -28,6 +29,8 @@ 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 */
>> +    short int nativeMode; /* 0=off, 1=tagged, 2=untagged */
>> +    unsigned int nativeTag;
>> };
>> 
>> void virNetDevVlanClear(virNetDevVlanPtr vlan);
>> diff --git a/tests/networkxml2xmlin/openvswitch-net.xml
>> b/tests/networkxml2xmlin/openvswitch-net.xml
>> index a3d82b1..2f6084d 100644
>> --- 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>
>> 
>> 
>> 
> 
> 
> Those are the only issues I see with this patch now.
> 
> Kyle - what is your opinion of implementing the config this way. Does it
> seem workable? Or was my ignorance of the subject matter showing when I
> suggested putting "nativeMode='tagged'" as an attribute of a tag (rather
> than putting "nativeTag" and "nativeMode" attributes directly in <vlan>,
> as James did in the first version of the patch)?

I think the approach you have here looks workable, as James indicated,
the approach you had him take with the subsequent versions of these
patches look more workable. I like having the nativeMode as an
attribute of the tag.

Also, the patch looks good to me!

Acked-by: Kyle Mestery <kmestery cisco com>


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