[libvirt] [PATCHv2 4/9] conf: support abstracted interface info in domain interface XML

[Laine Stump]

On 07/20/2011 05:54 PM, Eric Blake wrote:
> On 07/20/2011 12:21 AM, Laine Stump wrote:
>
>>
>> <p>
>> -      Provides a virtual network using a bridge device in the host.
> ...
>> -      overridden with the<target> element (see
>> +
>> +      Provides a connection whose details are described by the named
>
> Why the blank line  between <p> and the start of the paragraph?


I usually put a few blank lines in between what I'm changing and the 
surrounding text to minimize the amount of changed lines due to 
re-flowing. I then close up the blank lines before I'm done. OR I don't 
- this time I forgot.


>
> [and I really hate it that thunderbird is back to its habits of 
> munging quoted text in my emails again - for a while there, I was 
> running a version where the problem went away, but the latest distro 
> upgrade made it resurface]

I've been very bothered lately by Thunderbird's belief that it's okay to 
mess around with my text however it sees fit. For example, I can't get 
it to honor extra indenting whitespace I put at the beginning of lines 
when I'm putting XML examples in my message.


>
>>
>> +void
>> +virDomainActualNetDefFree(virDomainActualNetDefPtr def)
>
> Add this to cfg.mk's list of free-like functions.
>
>> +{
>> +    if (!def)
>> +        return;
>> +
>> +    switch (def->type) {
>> +    case VIR_DOMAIN_NET_TYPE_BRIDGE:
>> +        VIR_FREE(def->data.bridge.brname);
>> +        break;
>> +    case VIR_DOMAIN_NET_TYPE_DIRECT:
>> +        VIR_FREE(def->data.direct.linkdev);
>> +        VIR_FREE(def->data.direct.virtPortProfile);
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +}
>
> Memory leak of def itself.


Derp!


>
>>
>> +static int
>> +virDomainActualNetDefParseXML(xmlNodePtr node,
>> +                              xmlXPathContextPtr ctxt,
>> +                              virDomainActualNetDefPtr *def)
>> +{
>> +    virDomainActualNetDefPtr actual = NULL;
>> +    int ret = -1;
>> +    xmlNodePtr save_ctxt = ctxt->node;
>> +    char *type = NULL;
>> +    char *mode = NULL;
>> +
>> +    if (VIR_ALLOC(actual)<  0) {
>> +        virReportOOMError();
>> +        return -1;
>> +    }
>> +
>> +    ctxt->node = node;
>> +
>> +    type = virXMLPropString(node, "type");
>> +    if (!type) {
>> +        virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                             _("missing type attribute in 
>> interface's<actual>  element"));
>> +        goto error;
>> +    }
>> +    if ((int)(actual->type = virDomainNetTypeFromString(type))<  0) {
>
> This cast is not needed if you tweak domain_conf.h.
>
>> +
>> +    *def = actual;
>> +    actual = NULL;
>> +    ret = 0;
>> +error:
>> +    VIR_FREE(type);
>> +    VIR_FREE(mode);
>> +
>> +    ctxt->node = save_ctxt;
>> +    return ret;
>
> Memory leak of actual on error cases.


Double derp! (This previously didn't leak because I allocated directly 
into the parent object, and let the caller's destructor cleanup on failure).


>
>> @@ -6772,7 +6887,8 @@ virDomainDefPtr 
>> virDomainDefParseNode(virCapsPtr caps,
>>       }
>>
>>       ctxt->node = root;
>> -    def = virDomainDefParseXML(caps, xml, root, ctxt, 
>> expectedVirtTypes, flags);
>> +    def = virDomainDefParseXML(caps, xml, root, ctxt, 
>> expectedVirtTypes,
>> +                               flags);
>
> Your change and then undo made for a spurious hunk.
>
>>
>>   cleanup:
>>       xmlXPathFreeContext(ctxt);
>> @@ -6803,7 +6919,8 @@ virDomainObjParseNode(virCapsPtr caps,
>>       }
>>
>>       ctxt->node = root;
>> -    obj = virDomainObjParseXML(caps, xml, ctxt, expectedVirtTypes, 
>> flags);
>> +    obj = virDomainObjParseXML(caps, xml, ctxt, expectedVirtTypes,
>> +                               flags);
>
> and another one.
>
>> @@ -10008,7 +10201,10 @@ int virDomainSaveStatus(virCapsPtr caps,
>>                           const char *statusDir,
>>                           virDomainObjPtr obj)
>>   {
>> -    unsigned int flags = 
>> VIR_DOMAIN_XML_SECURE|VIR_DOMAIN_XML_INTERNAL_STATUS;
>> +    unsigned int flags = (VIR_DOMAIN_XML_SECURE |
>> +                          VIR_DOMAIN_XML_INTERNAL_STATUS |
>> +                          VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET);
>> +
>>       int ret = -1;
>>       char *xml;
>>
>> @@ -10099,7 +10295,8 @@ static virDomainObjPtr 
>> virDomainLoadStatus(virCapsPtr caps,
>>           goto error;
>>
>>       if (!(obj = virDomainObjParseFile(caps, statusFile, 
>> expectedVirtTypes,
>> -                                      VIR_DOMAIN_XML_INTERNAL_STATUS)))
>> +                                      VIR_DOMAIN_XML_INTERNAL_STATUS |
>> +                                      
>> VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET)))
>
> Thinking out loud here - it looks like we never use _INTERNAL_STATUS 
> or _INTERNAL_ACTUAL_NET in isolation - as of this patch, it is always 
> both or neither.  Maybe we could have let _INTERNAL_STATUS be the key 
> on whether to also output/parse <actual> elements rather than adding a 
> new flag.  On the other hand, if we ever change our mind and decide 
> that it makes sense to let the user do 'virsh dumpxml dom --actual' to 
> see which resources actually got used, then it is easier to change 
> VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET into a public flag, than it is if 
> we lump all internal actions under a single _INTERNAL_STATUS flags.  
> So no change necessary for now.
>
>> +++ b/src/conf/domain_conf.h
>> @@ -343,6 +343,27 @@ enum virDomainNetVirtioTxModeType {
>>       VIR_DOMAIN_NET_VIRTIO_TX_MODE_LAST,
>>   };
>>
>> +/* Config that was actually used to bring up interface, after
>> + * resolving network reference. This is private data, only used within
>> + * libvirt, but still must maintain backward compatibility, because
>> + * different versions of libvirt may read the same data file.
>> + */
>> +typedef struct _virDomainActualNetDef virDomainActualNetDef;
>> +typedef virDomainActualNetDef *virDomainActualNetDefPtr;
>> +struct _virDomainActualNetDef {
>> +    enum virDomainNetType type;
>
> Per the earlier cast comment, use int here, to match most other 
> _virDomain*Def typed unions.


Okay. But the original struct this was copied from (virDomainNetDef) 
uses the enum directly.


>
>> @@ -743,7 +749,6 @@ virNetworkSaveConfig;
>>   virNetworkSetBridgeMacAddr;
>>   virNetworkSetBridgeName;
>>
>> -
>>   # node_device_conf.h
>
> A spurious whitespace change.
>
>> +++ b/src/libxl/libxl_driver.c
>> @@ -707,7 +707,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, 
>> virDomainObjPtr vm,
>>       }
>>
>>       vm->def->id = domid;
>> -    if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL)
>> +    if ((dom_xml = virDomainDefFormat(vm->def)) == NULL)
>
> Oops, that's a compile failure, caused by over-undoing your 
> now-abandoned idea of adding a parameter.
>
> ACK with this squashed in:

Squashed and pushed. Thanks!