Re: [libvirt] [PATCH v3 03/13] Turn virDomainObjList into an opaque virObject

4 Feb 2013

On Fri, Feb 01, 2013 at 11:18:25 +0000, Daniel P. Berrange wrote:
...
From: "Daniel P. Berrange" <berrange@redhat.com>
As a step towards making virDomainObjList thread-safe turn it
into an opaque virObject, preventing any direct access to its
internals.
As part of this a new method virDomainObjListForEach is
introduced to replace all existing usage of virHashForEach
...

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 990e6e4..3861e68 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
...
@@ -15224,6 +15241,34 @@ cleanup:
     return -1;
 }
+
+struct virDomainListIterData {
+    virDomainObjListIterator callback;
+    void *opaque;
+    int ret;
+};
+
+static void virDomainObjListHelper(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque)
static void
    virDomainObjListHelper(void *payload,
                           const void *name ATTRIBUTE_UNUSED,
                           void *opaque)

would better fit out style and would not be longer then 80 chars :-)
...
+{
+    struct virDomainListIterData *data = opaque;
+
+    if (data->callback(payload, data->opaque) < 0)
+        data->ret = -1;
+}
+
+int virDomainObjListForEach(virDomainObjListPtr doms,
+                            virDomainObjListIterator callback,
+                            void *opaque)
+{
+    struct virDomainListIterData data = {
+        callback, opaque, 0,
+    };
+    virHashForEach(doms->objs, virDomainObjListHelper, &data);
+
+    return data.ret;
+}
+
+
 int virDomainChrDefForeach(virDomainDefPtr def,
                            bool abortOnError,
                            virDomainChrDefIterator iter,
...
diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
index 35f8dde..b4573f5 100644
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
...
@@ -725,14 +725,14 @@ void virNWFilterObjUnlock(virNWFilterObjPtr obj);
 void virNWFilterLockFilterUpdates(void);
 void virNWFilterUnlockFilterUpdates(void);
-int virNWFilterConfLayerInit(virHashIterator domUpdateCB);
+int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB);
 void virNWFilterConfLayerShutdown(void);
int virNWFilterInstFiltersOnAllVMs(virConnectPtr conn);
typedef int (*virNWFilterRebuild)(virConnectPtr conn,
-                                  virHashIterator, void *data);
+                                  virDomainObjListIterator, void *data);
While changing this line, you could remove this mixture of styles for
function prototypes:

    ...
    virDomainObjListIterator domUpdateCB,
    void *data);
...
typedef void (*virNWFilterVoidCall)(void);
...
...
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index 46f30ca..6eacbca 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
...
@@ -594,35 +595,20 @@ int openvzLoadDomains(struct openvz_driver *driver) {
         }
         *line++ = '\0';
-        if (!(dom = virDomainObjNew(driver->caps)))
-             goto cleanup;
-
-        if (VIR_ALLOC(dom->def) < 0)
+        if (VIR_ALLOC(def) < 0)
             goto no_memory;
-        dom->def->virtType = VIR_DOMAIN_VIRT_OPENVZ;
-
-        if (STREQ(status, "stopped")) {
-            virDomainObjSetState(dom, VIR_DOMAIN_SHUTOFF,
-                                 VIR_DOMAIN_SHUTOFF_UNKNOWN);
-        } else {
-            virDomainObjSetState(dom, VIR_DOMAIN_RUNNING,
-                                 VIR_DOMAIN_RUNNING_UNKNOWN);
-        }
+        def->virtType = VIR_DOMAIN_VIRT_OPENVZ;
-        dom->pid = veid;
         if (virDomainObjGetState(dom, NULL) == VIR_DOMAIN_SHUTOFF)
-            dom->def->id = -1;
+            def->id = -1;
         else
-            dom->def->id = veid;
-        /* XXX OpenVZ doesn't appear to have concept of a transient domain */
-        dom->persistent = 1;
-
-        if (virAsprintf(&dom->def->name, "%i", veid) < 0)
+            def->id = veid;
You moved all the code that creates dom further but left this one here.
I think you actually wanted to change

    if (virDomainObjGetState(dom, NULL) == VIR_DOMAIN_SHUTOFF)
        def->id = -1;
    else
        def->id = veid;

to

    if (STREQ(status, "stopped"))
        def->id = -1;
    else
        def->id = veid;
...
+        if (virAsprintf(&def->name, "%i", veid) < 0)
             goto no_memory;
openvzGetVPSUUID(veid, uuidstr, sizeof(uuidstr));
-        ret = virUUIDParse(uuidstr, dom->def->uuid);
+        ret = virUUIDParse(uuidstr, def->uuid);
...
+        if (!(dom = virDomainObjListAdd(driver->domains,
+                                        driver->caps,
+                                        def,
+                                        STRNEQ(status, "stopped"))))
             goto cleanup;
+
+        if (STREQ(status, "stopped")) {
+            virDomainObjSetState(dom, VIR_DOMAIN_SHUTOFF,
+                                 VIR_DOMAIN_SHUTOFF_UNKNOWN);
+        } else {
+            virDomainObjSetState(dom, VIR_DOMAIN_RUNNING,
+                                 VIR_DOMAIN_RUNNING_UNKNOWN);
         }
+        /* XXX OpenVZ doesn't appear to have concept of a transient domain */
+        dom->persistent = 1;
+        dom->pid = veid;
Shouldn't dom->pid be set only if the domain is running?
...
virObjectUnlock(dom);
         dom = NULL;
+        def = NULL;
     }
virCommandFree(cmd);
...

ACK with the small issues fixed.

Jirka

    

Re: [libvirt] [PATCH v3 03/13] Turn virDomainObjList into an opaque virObject

Jiri Denemark