On 02/14/2013 05:00 AM, Stefan Berger wrote:
Extend the QEMU private domain structure with virFdSet. Persist the virFdSet using XML and parse its XML. Reset the FdSet upon domain stop.
Stefan Berger <stefanb@linux.vnet.ibm.com>
--- v5->v6: - change found in patch 3 moved to this patch
@@ -470,6 +480,9 @@ static int qemuDomainObjPrivateXMLParse(
priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1;
+ if (virFdSetParseXML(priv->fdset, "./fdset/entry", ctxt) < 0) + goto error;
Does this work on the upgrade path? If older libvirt did not use fdsets, then the XML element will not be present, but in patch 1, you had: + if ((n = virXPathNodeSet(xPath, ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to parse qemu file descriptor sets")); + goto error; + } which implies an error if there was nothing to parse. But in reality, nothing to parse should be treated the same as success - there's no set to reinstate, because it was from an older libvirt that didn't use a set. /me rereads patch 1... Ewww - this works, but only by accident. In patch 1, you initialized int ret = 0, therefore, the 'error:' label is reached with ret still at 0, and the function returns success. Typically, we name a label 'cleanup:' rather than 'error:' when the label can be used on the success path; also, we tend to initialize ret to -1 and then flip it to 0 just before cleanup, instead of your approach of initializing to 0 and flipping to -1 on all call sites that goto error because of a real problem.
+++ libvirt/src/qemu/qemu_process.c @@ -4255,6 +4255,8 @@ void qemuProcessStop(virQEMUDriverPtr dr priv->monConfig = NULL; }
+ virFdSetReset(priv->fdset);
An offline domain doesn't need an fdset. I think that rather than pre-allocate the set when creating the vm, then resetting it, that instead we create it just before starting a domain, and free the set when stopping the domain: virObjectUnref(priv->fdset); priv->fdset = NULL; so that we aren't wasting memory for a set with offline domains, since offline domains also have no aliases that would live in an fdset. And if we do that, then my complaint on 1/3 about not needing a public virFdSetReset() is valid, as this appears to be the only place you do a reset. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org