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

Re: [libvirt] PATCH: 2/5: Locking on virDomainObjPtr objects



This patch provides for locking of the virDomainObjPtr instances on a per
object level, and defines locking semantics of the various API calls 
that use these objects.

Since this locking only covers the individual objects, it is expected that
the driver provide a higher level of locking around arrays of objects.
I considering putting an explicit lock on the virDomainObjListPtr array,
but this adds more complexity in the drivers, and does not improve 
safety or concurrency.


 * Domain lookup methods

  virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms,
                                    int id);
  virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms,
                                      const unsigned char *uuid);
  virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms,
                                      const char *name);

  The driver must hold its global lock to protect the virDomainObjListPtr
  object. The returned virDomainObjPtr instance will be locked at which
  point the driver  can optionall release its global lock

 * Creating virDomainObjPtr instances

  virDomainObjPtr virDomainAssignDef(virConnectPtr conn,
                                     virDomainObjListPtr doms,
                                     const virDomainDefPtr def);

   The driver must hold its global lock to protect the virDomainObjListPtr
   object. The newly created or cached virDomainObjPtr instance that is
   returned will be locked.

 * Removing a virDomainObjPtr instsance:

  void virDomainRemoveInactive(virDomainObjListPtr doms,
                               virDomainObjPtr dom);

   The driver must hold its global lock to protect the virDomaiNObjListPtr
   object. The virDomainObjPtr must be unlocked.


For all the other methods in domain_conf.h, which deal with the config
in the virDomainDefPtr instances, the caller must hold a lock over the
virDomainObjPtr instance which owns the virDomainDefPtr.


This may all sound fairly onerous / complex, but it is straightforward
to work with in the drivers, as the following patches will demonstrate

Daniel

diff --git a/src/domain_conf.c b/src/domain_conf.c
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -145,40 +145,70 @@ VIR_ENUM_IMPL(virDomainHostdevSubsys, VI
         __virReportErrorHelper(conn, VIR_FROM_DOMAIN, code, __FILE__,        \
                                __FUNCTION__, __LINE__, fmt)
 
+/**
+ * @virDomainFindByID:
+ *
+ * Caller must hold exclusive lock over 'doms'
+ *
+ * Returns a virDomainObjPtr locked for exclusive access, or NULL
+ */
 virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms,
                                   int id)
 {
     unsigned int i;
 
-    for (i = 0 ; i < doms->count ; i++)
+    for (i = 0 ; i < doms->count ; i++) {
+        virDomainLock(doms->objs[i]);
         if (virDomainIsActive(doms->objs[i]) &&
             doms->objs[i]->def->id == id)
             return doms->objs[i];
+        virDomainUnlock(doms->objs[i]);
+    }
 
     return NULL;
 }
 
 
+/**
+ * @virDomainFindByUUID:
+ *
+ * Caller must hold exclusive lock over 'doms'
+ *
+ * Returns a virDomainObjPtr locked for exclusive access, or NULL
+ */
 virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms,
                                     const unsigned char *uuid)
 {
     unsigned int i;
 
-    for (i = 0 ; i < doms->count ; i++)
+    for (i = 0 ; i < doms->count ; i++) {
+        virDomainLock(doms->objs[i]);
         if (!memcmp(doms->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN))
             return doms->objs[i];
+        virDomainUnlock(doms->objs[i]);
+    }
 
     return NULL;
 }
 
+/**
+ * @virDomainFindByName:
+ *
+ * Caller must hold exclusive lock over 'doms'
+ *
+ * Returns a virDomainObjPtr locked for exclusive access, or NULL
+ */
 virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms,
                                     const char *name)
 {
     unsigned int i;
 
-    for (i = 0 ; i < doms->count ; i++)
+    for (i = 0 ; i < doms->count ; i++) {
+        virDomainLock(doms->objs[i]);
         if (STREQ(doms->objs[i]->def->name, name))
             return doms->objs[i];
+        virDomainUnlock(doms->objs[i]);
+    }
 
     return NULL;
 }
@@ -406,6 +436,12 @@ void virDomainDefFree(virDomainDefPtr de
     VIR_FREE(def);
 }
 
+/**
+ * @virDomainObjListFree
+ *
+ * Caller should not hold lock on 'dom', but must
+ * also ensure no other thread can have an active lock
+ */
 void virDomainObjFree(virDomainObjPtr dom)
 {
     if (!dom)
@@ -419,17 +455,35 @@ void virDomainObjFree(virDomainObjPtr do
     VIR_FREE(dom);
 }
 
-void virDomainObjListFree(virDomainObjListPtr vms)
+/**
+ * @virDomainObjListFree
+ *
+ * Caller must hold exclusive lock over 'doms'
+ */
+void virDomainObjListFree(virDomainObjListPtr doms)
 {
     unsigned int i;
 
-    for (i = 0 ; i < vms->count ; i++)
-        virDomainObjFree(vms->objs[i]);
+    for (i = 0 ; i < doms->count ; i++) {
+        /* Lock+unlock ensures no one is still using this dom */
+        virDomainLock(doms->objs[i]);
+        virDomainUnlock(doms->objs[i]);
 
-    VIR_FREE(vms->objs);
-    vms->count = 0;
+        virDomainObjFree(doms->objs[i]);
+    }
+
+    VIR_FREE(doms->objs);
+    doms->count = 0;
 }
 
+
+/**
+ * @virDomainAssignDef
+ *
+ * Caller must hold exclusive lock over 'doms'.
+ *
+ * The return domain object will be locked
+ */
 virDomainObjPtr virDomainAssignDef(virConnectPtr conn,
                                    virDomainObjListPtr doms,
                                    const virDomainDefPtr def)
@@ -456,6 +510,7 @@ virDomainObjPtr virDomainAssignDef(virCo
 
     domain->state = VIR_DOMAIN_SHUTOFF;
     domain->def = def;
+    pthread_mutex_init(&domain->lock, NULL);
 
     if (VIR_REALLOC_N(doms->objs, doms->count + 1) < 0) {
         virDomainReportError(conn, VIR_ERR_NO_MEMORY, NULL);
@@ -466,13 +521,26 @@ virDomainObjPtr virDomainAssignDef(virCo
     doms->objs[doms->count] = domain;
     doms->count++;
 
+    virDomainLock(domain);
+
     return domain;
 }
 
+
+/**
+ * @virDomainRemoveInactive:
+ *
+ * Caller must hold exclusive lock over 'doms', but
+ * 'dom' must be unlocked
+ */
 void virDomainRemoveInactive(virDomainObjListPtr doms,
                              virDomainObjPtr dom)
 {
     unsigned int i;
+
+    /* Ensure no other thread holds a lock on dom */
+    virDomainLock(dom);
+    virDomainUnlock(dom);
 
     for (i = 0 ; i < doms->count ; i++) {
         if (doms->objs[i] == dom) {
@@ -3335,8 +3403,10 @@ int virDomainLoadAllConfigs(virConnectPt
                                   configDir,
                                   autostartDir,
                                   entry->d_name);
-        if (dom)
+        if (dom) {
             dom->persistent = 1;
+            virDomainUnlock(dom);
+        }
     }
 
     closedir(dir);
diff --git a/src/domain_conf.h b/src/domain_conf.h
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -452,6 +452,8 @@ typedef struct _virDomainObj virDomainOb
 typedef struct _virDomainObj virDomainObj;
 typedef virDomainObj *virDomainObjPtr;
 struct _virDomainObj {
+    pthread_mutex_t lock;
+
     int stdin_fd;
     int stdout_fd;
     int stderr_fd;
@@ -481,6 +483,20 @@ virDomainIsActive(virDomainObjPtr dom)
 virDomainIsActive(virDomainObjPtr dom)
 {
     return dom->def->id != -1;
+}
+
+
+static inline int
+virDomainLock(virDomainObjPtr dom)
+{
+    return pthread_mutex_lock(&dom->lock);
+}
+
+
+static inline int
+virDomainUnlock(virDomainObjPtr dom)
+{
+    return pthread_mutex_unlock(&dom->lock);
 }
 
 

-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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