6:49 p.m.
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!