On Tue, Feb 05, 2013 at 14:48:01 +0000, Daniel P. Berrange wrote:
On Mon, Feb 04, 2013 at 04:58:25PM +0100, Jiri Denemark wrote:
On Fri, Feb 01, 2013 at 11:18:26 +0000, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The duplicate VM checking should be done atomically with virDomainObjListAdd, so shoud not be a separate function. Instead just use flags to indicate what kind of checks are required.
...
This pair, used in virDomainDefinXML: @@ -12623,7 +12601,10 @@ static virDomainPtr qemuDomainAttach(virConnectPtr conn,
if (!(vm = virDomainObjListAdd(driver->domains, driver->caps, - def, false))) + def, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | + VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, + NULL)))
Although this could be correct, it doesn't match current code. To match it, you'd need to use just VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE flag.
Yep, I believe this new code is correct
Looking forward to v2 :-)
Here is the diff from v1 to v2
Daniel
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1771668..06dbe1f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1853,21 +1853,33 @@ error:
void virDomainObjAssignDef(virDomainObjPtr domain, const virDomainDefPtr def, - bool live) + bool live, + virDomainDefPtr *oldDef) { - if (!virDomainObjIsActive(domain)) { + *oldDef = NULL;
Oops, oldDef is optional; the above should be + if (oldDef) + *oldDef = NULL;
+ if (virDomainObjIsActive(domain)) { + if (oldDef) + *oldDef = domain->newDef; + else + virDomainDefFree(domain->newDef); + domain->newDef = def; + } else { if (live) { - /* save current configuration to be restored on domain shutdown */ - if (!domain->newDef) - domain->newDef = domain->def; + if (domain->def) {
This test is not really needed since if both def and newDef are NULL, newDef = def does nothing and if def is NULL but newDef is not, virDomainFree should be able to handle NULL argument. Anyway, it's not a big deal, just that the diff could have been smaller :-)
+ /* save current configuration to be restored on domain shutdown */ + if (!domain->newDef) + domain->newDef = domain->def; + else + virDomainDefFree(domain->def); + } domain->def = def; } else { ...
ACK to the original patch with these changes (after fixing the possible NULL dereference) squashed in. Jirka