[libvirt] [PATCH 07/10] network: new XML to support virtual switch functionality

[Laine Stump]

On 07/08/2011 04:02 PM, Eric Blake wrote:
> On 07/05/2011 01:45 AM, Laine Stump wrote:
>
>> +        virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>> +                             _("unknown type '%s' in interface's<actual>  element"), type);
>> +        goto error;
>> +    }
>> +    if ((*actual)->type != VIR_DOMAIN_NET_TYPE_BRIDGE&&
>> +        (*actual)->type != VIR_DOMAIN_NET_TYPE_DIRECT) {
> This is a lot of dereferencing through *actual.  It might be easier to
> delay the dereferencing to the very end,


Yeah, at the time it seemed like it was going to be easier to cleanup 
this way (the caller is already going to free the ActualNetDef anyway), 
but in practice it didn't work out. I changed it to the more standard way.


>> +
>> +    if ((*actual)->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>> +
>> +        (*actual)->data.bridge.brname = virXPathString("string(./source[1]/@bridge)",
>> +                                                       ctxt);
>> +
>> +    } else if ((*actual)->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
> Nit: those two blank lines don't quite match style elsewhere in the file.


Fixed.


>> @@ -9871,7 +10063,9 @@ int virDomainSaveStatus(virCapsPtr caps,
>>                           const char *statusDir,
>>                           virDomainObjPtr obj)
>>   {
>> -    int flags = VIR_DOMAIN_XML_SECURE|VIR_DOMAIN_XML_INTERNAL_STATUS;
>> +    int flags = VIR_DOMAIN_XML_SECURE |
>> +       VIR_DOMAIN_XML_INTERNAL_STATUS |
>> +       VIR_DOMAIN_XML_ACTUAL_NET;
> This indentation looks unusual (7 spaces, but no prior hanging '(' to be
> flush with).  I tend to write expressions like this as:
>
>      int flags = (VIR_DOMAIN_XML_SECURE |
>                   VIR_DOMAIN_XML_INTERNAL_STATUS |
>                   VIR_DOMAIN_XML_ACTUAL_NET);
>
> Also, there may be some merge conflicts if my patch series for cleaning
> up flags usage goes in first.


Indentation fixed. (And thanks for all the flags work, btw :-)


>> +++ b/src/conf/domain_conf.h
>> @@ -1,3 +1,4 @@
>> +
>>   /*
> Why the spurious addition of a blank leading line?


I likely hit the enter key by accident just after opening the file, and 
didn't notice. I removed it.


>> +        case VIR_NETWORK_FORWARD_PASSTHROUGH:
>> +            if (def->bridge) {
>> +                virNetworkReportError(VIR_ERR_XML_ERROR,
>> +                                      _("bridge name not allowed in %s mode (network '%s'"),
>> +                                      virNetworkForwardTypeToString(def->forwardType),
>> +                                      def->name);
>> +                goto error;
>> +            }
>> +            /* Fall through to next case */
> I'm not sure whether Coverity will recognize that spelling, so here's
> hoping.  A quick 'git grep -iC2 "fall.\\?thr"' found existing spellings
> that Coverity appears to honor:
>
> /* fallthrough */
> /* fall through */
> /* Fallthrough */
>
> but I'd have to actually check Coverity source to see what the canonical
> list is.


Yeesh. "Fallthrough"? That sounds like a noun! I changed it to "fall 
through" to be sure.


>> +        /* Duplicate the first interface from the pool into<forward
>> +         * dev=xxx for convenience.
> Missing the paired>  in the comment.

Fixed.