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

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



On 08/14/2012 07:08 PM, Eric Blake wrote:
> 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>.

Fixed.

>
>> +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.

Removed.

>
>> +    /* 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.

Okay. I changed it to quietly ignore if someone puts "trunk='no'" when
nTags == 1, but log an error otherwise (unless it's trunk='yes' of
course - that's always okay.)

> ACK with nits addressed.

I'll push it after I heard the results of my question about 2/4


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