[libvirt] [PATCHv2 3/4] conf: add <vlan> element to network and domain interface elements

[Eric Blake]

On 08/14/2012 01:15 AM, Laine Stump wrote:
> The following config elements now support a <vlan> subelements:
> 
> within a domain: <interface>, and the <actual> subelement of <interface>
> within a network: the toplevel, as well as any <portgroup>
> 
> Each vlan element must have one or more <tag id='n'/> subelements.  If
> there is more than one tag, it is assumed that vlan trunking is being
> requested. If trunking is required with only a single tag, the
> attribute "trunk='yes'" should be added to the toplevel <vlan>
> element.
> 
> 
> IMPORTANT NOTE: As of this patch there is no backend support for the
> vlan element for *any* network device type. When support is added in
> later patches, it will only be for those select network types that
> support setting up a vlan on the host side, without the guest's
> involvement. (For example, it will be possible to configure a vlan for
> a guest connected to an openvswitch bridge, but it won't be possible

> +        <element name="tag">
> +          <attribute name="id">
> +            <data type="unsignedInt">
> +              <param name="maxInclusive">4095</param>
> +            </data>
> +          </attribute>

I would also add <empty/> here, since the user should always be using
<tag/> rather than <tag><sub/></tag> or <tag>data</tag>.

> +int
> +virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr def)
> +{
> +    int ret = -1;
> +    xmlNodePtr save = ctxt->node;
> +    const char *trunk;
> +    xmlNodePtr *tagNodes = NULL;
> +    int nTags;
> +
> +    ctxt->node = node;
> +
> +    nTags = virXPathNodeSet("./tag", ctxt, &tagNodes);
> +    if (nTags < 0)
> +        goto error;
> +
> +    if (nTags == 0) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("missing tag id - each <vlan> must have "
> +                         "at least one <tag id='n'/> subelement"));
> +        goto error;
> +    }
> +
> +    if (nTags > 0) {

This 'if' is spurious, given the above two statements always
short-circuiting to error.  Cosmetic, though.

> +    /* now that we know how many tags there are, look for an explicit
> +     * trunk setting.
> +     */
> +    if (nTags > 1)
> +        def->trunk = true;
> +
> +    ctxt->node = node;
> +    if ((trunk = virXPathString("string(./@trunk)", ctxt)) != NULL) {
> +        def->trunk = STRCASEEQ(trunk, "yes");
> +        if (nTags > 1 && !def->trunk) {

It took me a couple reads to see - this says the user can pass
trunk='garbage' for a single <tag> (it won't get past the RNG
validation, but our C code doesn't care), if they passed trunk at all,
it MUST be trunk='yes' for multiple <tag>s.  Okay.

ACK with nits addressed.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120814/b1b547d2/attachment-0001.sig>