[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 04/13] Merge virDomainObjListIsDuplicate into virDomainObjListAdd



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;
+    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) {
+                /* save current configuration to be restored on domain shutdown */
+                if (!domain->newDef)
+                    domain->newDef = domain->def;
+                else
+                    virDomainDefFree(domain->def);
+            }
             domain->def = def;
         } else {
-            virDomainDefFree(domain->def);
+            if (oldDef)
+                *oldDef = domain->def;
+            else
+                virDomainDefFree(domain->def);
             domain->def = def;
         }
-    } else {
-        virDomainDefFree(domain->newDef);
-        domain->newDef = def;
     }
 }
 
@@ -1879,6 +1891,10 @@ void virDomainObjAssignDef(virDomainObjPtr domain,
  * this will refuse updating an existing def if the
  * current def is Live
  *
+ * If flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE then
+ * the @def being added is assumed to represent a
+ * live config, not a future inactive config
+ *
  */
 virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
                                     virCapsPtr caps,
@@ -1899,7 +1915,7 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
             virReportError(VIR_ERR_OPERATION_FAILED,
                            _("domain '%s' is already defined with uuid %s"),
                            vm->def->name, uuidstr);
-            goto cleanup;
+            goto error;
         }
 
         if (flags & VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE) {
@@ -1908,30 +1924,14 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
                 virReportError(VIR_ERR_OPERATION_INVALID,
                                _("domain is already active as '%s'"),
                                vm->def->name);
-                goto cleanup;
+                goto error;
             }
         }
 
-        if (virDomainObjIsActive(vm)) {
-            if (oldDef)
-                *oldDef = vm->newDef;
-            else
-                virDomainDefFree(vm->newDef);
-            vm->newDef = def;
-        } else {
-            if (flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE) {
-                if (!vm->newDef)
-                    vm->newDef = vm->def;
-                else
-                    virDomainDefFree(vm->newDef);
-            } else {
-                if (oldDef)
-                    *oldDef = vm->def;
-                else
-                    virDomainDefFree(vm->def);
-            }
-            vm->def = def;
-        }
+        virDomainObjAssignDef(vm,
+                              def,
+                              !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE),
+                              oldDef);
     } else {
         /* UUID does not match, but if a name matches, refuse it */
         if ((vm = virDomainObjListFindByName(doms, def->name))) {
@@ -1939,9 +1939,7 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
             virReportError(VIR_ERR_OPERATION_FAILED,
                            _("domain '%s' already exists with uuid %s"),
                            def->name, uuidstr);
-            virObjectUnlock(vm);
-            vm = NULL;
-            goto cleanup;
+            goto error;
         }
 
         if (!(vm = virDomainObjNew(caps)))
@@ -1950,12 +1948,17 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
 
         virUUIDFormat(def->uuid, uuidstr);
         if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) {
-            VIR_FREE(vm);
+            virObjectUnref(vm);
             return NULL;
         }
     }
 cleanup:
     return vm;
+
+error:
+    virObjectUnlock(vm);
+    vm = NULL;
+    goto cleanup;
 }
 
 /*
@@ -14879,7 +14882,7 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms,
     virDomainDefPtr def = NULL;
     virDomainObjPtr dom;
     int autostart;
-    int newVM = 1;
+    virDomainDefPtr oldDef = NULL;
 
     if ((configFile = virDomainConfigFile(configDir, name)) == NULL)
         goto error;
@@ -14893,32 +14896,15 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms,
     if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
         goto error;
 
-    /* if the domain is already in our hashtable, we only need to
-     * update the autostart flag
-     */
-    if ((dom = virDomainObjListFindByUUID(doms, def->uuid))) {
-        dom->autostart = autostart;
-
-        if (virDomainObjIsActive(dom) &&
-            !dom->newDef) {
-            virDomainObjAssignDef(dom, def, false);
-        } else {
-            virDomainDefFree(def);
-        }
-
-        VIR_FREE(configFile);
-        VIR_FREE(autostartLink);
-        return dom;
-    }
-
-    if (!(dom = virDomainObjListAdd(doms, caps, def, 0, NULL)))
+    if (!(dom = virDomainObjListAdd(doms, caps, def, 0, &oldDef)))
         goto error;
 
     dom->autostart = autostart;
 
     if (notify)
-        (*notify)(dom, newVM, opaque);
+        (*notify)(dom, oldDef == NULL, opaque);
 
+    virDomainDefFree(oldDef);
     VIR_FREE(configFile);
     VIR_FREE(autostartLink);
     return dom;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index fa13e24..dc411e4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1966,8 +1966,6 @@ void virDomainDefFree(virDomainDefPtr vm);
 
 virDomainChrDefPtr virDomainChrDefNew(void);
 
-/* live == true means def describes an active domain (being migrated or
- * restored) as opposed to a new persistent configuration of the domain */
 enum {
     VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0),
     VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1),
@@ -1979,7 +1977,8 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
                                     virDomainDefPtr *oldDef);
 void virDomainObjAssignDef(virDomainObjPtr domain,
                            const virDomainDefPtr def,
-                           bool live);
+                           bool live,
+                           virDomainDefPtr *oldDef);
 int virDomainObjSetDefTransient(virCapsPtr caps,
                                 virDomainObjPtr domain,
                                 bool live);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index e0a9a8e..d11acfc 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -902,7 +902,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
                 goto error;
             }
 
-            virDomainObjAssignDef(vm, def, true);
+            virDomainObjAssignDef(vm, def, true, NULL);
             def = NULL;
 
             if (unlink(managed_save_path) < 0) {
@@ -3602,7 +3602,7 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
     if (!ret && (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) {
         ret = virDomainSaveConfig(driver->configDir, vmdef);
         if (!ret) {
-            virDomainObjAssignDef(vm, vmdef, false);
+            virDomainObjAssignDef(vm, vmdef, false, NULL);
             vmdef = NULL;
         }
     }
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 5e664c7..9f636e0 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1863,7 +1863,7 @@ lxcSetSchedulerParametersFlags(virDomainPtr dom,
         if (rc < 0)
             goto cleanup;
 
-        virDomainObjAssignDef(vm, vmdef, false);
+        virDomainObjAssignDef(vm, vmdef, false, NULL);
         vmdef = NULL;
     }
 
@@ -4411,7 +4411,7 @@ lxcDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
     if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
         ret = virDomainSaveConfig(driver->configDir, vmdef);
         if (!ret) {
-            virDomainObjAssignDef(vm, vmdef, false);
+            virDomainObjAssignDef(vm, vmdef, false, NULL);
             vmdef = NULL;
         }
     }
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index e9724fd..3081417 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -586,6 +586,7 @@ int openvzLoadDomains(struct openvz_driver *driver) {
 
     line = outbuf;
     while (line[0] != '\0') {
+        unsigned int flags = 0;
         if (virStrToLong_i(line, &status, 10, &veid) < 0 ||
             *status++ != ' ' ||
             (line = strchr(status, '\n')) == NULL) {
@@ -642,12 +643,14 @@ int openvzLoadDomains(struct openvz_driver *driver) {
         openvzReadMemConf(def, veid);
 
         virUUIDFormat(def->uuid, uuidstr);
+        flags = VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE;
+        if (STRNEQ(status, "stopped"))
+            flags |= VIR_DOMAIN_OBJ_LIST_ADD_LIVE;
+
         if (!(dom = virDomainObjListAdd(driver->domains,
                                         driver->caps,
                                         def,
-                                        VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE |
-                                        (STRNEQ(status, "stopped") ?
-                                         VIR_DOMAIN_OBJ_LIST_ADD_LIVE : 0),
+                                        flags,
                                         NULL)))
             goto cleanup;
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d6c6af5..b0683c6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5168,7 +5168,7 @@ qemuDomainObjRestore(virConnectPtr conn,
         goto cleanup;
     }
 
-    virDomainObjAssignDef(vm, def, true);
+    virDomainObjAssignDef(vm, def, true, NULL);
     def = NULL;
 
     ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path,
@@ -5611,8 +5611,7 @@ static virDomainPtr qemuDomainDefine(virConnectPtr conn, const char *xml) {
     if (!(vm = virDomainObjListAdd(driver->domains,
                                    driver->caps,
                                    def,
-                                   VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
-                                   VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
+                                   0,
                                    &oldDef)))
         goto cleanup;
 
@@ -5620,7 +5619,7 @@ static virDomainPtr qemuDomainDefine(virConnectPtr conn, const char *xml) {
     if (virDomainHasDiskMirror(vm)) {
         virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
                        _("domain has active block copy job"));
-        virDomainObjAssignDef(vm, NULL, false);
+        virDomainObjAssignDef(vm, NULL, false, NULL);
         goto cleanup;
     }
     vm->persistent = 1;
@@ -6532,7 +6531,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
     if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
         ret = virDomainSaveConfig(cfg->configDir, vmdef);
         if (!ret) {
-            virDomainObjAssignDef(vm, vmdef, false);
+            virDomainObjAssignDef(vm, vmdef, false, NULL);
             vmdef = NULL;
         }
     }
@@ -8045,7 +8044,7 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom,
         if (rc < 0)
             goto cleanup;
 
-        virDomainObjAssignDef(vm, vmdef, false);
+        virDomainObjAssignDef(vm, vmdef, false, NULL);
         vmdef = NULL;
     }
 
@@ -12181,13 +12180,13 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
                 goto endjob;
             }
             if (config)
-                virDomainObjAssignDef(vm, config, false);
+                virDomainObjAssignDef(vm, config, false, NULL);
         } else {
             /* Transitions 2, 3 */
         load:
             was_stopped = true;
             if (config)
-                virDomainObjAssignDef(vm, config, false);
+                virDomainObjAssignDef(vm, config, false, NULL);
 
             rc = qemuProcessStart(snapshot->domain->conn,
                                   driver, vm, NULL, -1, NULL, snap,
@@ -12271,7 +12270,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
             goto endjob;
         }
         if (config)
-            virDomainObjAssignDef(vm, config, false);
+            virDomainObjAssignDef(vm, config, false, NULL);
 
         if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
                      VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
index 8684f56..b7905fa 100644
--- a/src/vmware/vmware_driver.c
+++ b/src/vmware/vmware_driver.c
@@ -338,7 +338,9 @@ vmwareDomainDefineXML(virConnectPtr conn, const char *xml)
     /* assign def */
     if (!(vm = virDomainObjListAdd(driver->domains,
                                    driver->caps,
-                                   vmdef, 0, NULL)))
+                                   vmdef,
+                                   VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
+                                   NULL)))
         goto cleanup;
 
     pDomain = vm->privateData;


-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]