Re: [libvirt] [PATCH v3 04/13] Merge virDomainObjListIsDuplicate into virDomainObjListAdd
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(a)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