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

Re: [libvirt] PATCH: 12/25: Remove global state from Inotify driver



The 'inotify' driver currently keeps all its state in a global static
variables. Not so great since we access this from all sorts of places
and its hard to guarentee thread safety. Also, we're using per-connection
FD watches to update this state, so if multiple Xen connections were
active, they'd all be updating the same globl state. So we move the
state into the virConnect object. This will increase memory usage if
a single process has multiple Xen connections open though, but not
by very much

 xen_inotify.c |  105 +++++++++++++++++++++++++++++++---------------------------
 xen_unified.h |    7 +++
 xs_internal.c |   33 ++++++++----------
 3 files changed, 79 insertions(+), 66 deletions(-)

Daniel

diff --git a/src/xen_inotify.c b/src/xen_inotify.c
--- a/src/xen_inotify.c
+++ b/src/xen_inotify.c
@@ -48,9 +48,6 @@
                                __FUNCTION__, __LINE__, fmt)
 
 #define LIBVIRTD_DOMAINS_DIR "/var/lib/xend/domains"
-static const char *configDir        = NULL;
-static int  useXenConfigCache = 0;
-static xenUnifiedDomainInfoListPtr configInfoList = NULL;
 
 struct xenUnifiedDriver xenInotifyDriver = {
     xenInotifyOpen, /* open */
@@ -121,6 +118,7 @@ xenInotifyXendDomainsDirLookup(virConnec
     virDomainPtr dom;
     const char *uuid_str;
     unsigned char rawuuid[VIR_UUID_BUFLEN];
+    xenUnifiedPrivatePtr priv = conn->privateData;
 
     /* xend is managing domains. we will get
     * a filename in the manner:
@@ -142,15 +140,15 @@ xenInotifyXendDomainsDirLookup(virConnec
         /* If we are here, the domain has gone away.
            search for, and create a domain from the stored
            list info */
-        for (i=0; i<configInfoList->count; i++) {
-            if (!memcmp(uuid, configInfoList->doms[i]->uuid, VIR_UUID_BUFLEN)) {
-                *name = strdup(configInfoList->doms[i]->name);
+        for (i = 0 ; i < priv->configInfoList->count ; i++) {
+            if (!memcmp(uuid, priv->configInfoList->doms[i]->uuid, VIR_UUID_BUFLEN)) {
+                *name = strdup(priv->configInfoList->doms[i]->name);
                 if (!*name) {
                     virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR,
                                        _("finding dom for %s"), uuid_str);
                     return -1;
                 }
-                memcpy(uuid, configInfoList->doms[i]->uuid, VIR_UUID_BUFLEN);
+                memcpy(uuid, priv->configInfoList->doms[i]->uuid, VIR_UUID_BUFLEN);
                 DEBUG0("Found dom on list");
                 return 0;
             }
@@ -172,7 +170,8 @@ xenInotifyDomainLookup(virConnectPtr con
 xenInotifyDomainLookup(virConnectPtr conn,
                        const char *filename,
                        char **name, unsigned char *uuid) {
-    if (useXenConfigCache)
+    xenUnifiedPrivatePtr priv = conn->privateData;
+    if (priv->useXenConfigCache)
         return xenInotifyXenCacheLookup(filename, name, uuid);
     else
         return xenInotifyXendDomainsDirLookup(conn, filename, name, uuid);
@@ -195,8 +194,9 @@ xenInotifyDomainEventFromFile(virConnect
 }
 
 static int
-xenInotifyXendDomainsDirRemoveEntry(virConnectPtr conn ATTRIBUTE_UNUSED,
+xenInotifyXendDomainsDirRemoveEntry(virConnectPtr conn,
                                     const char *fname) {
+    xenUnifiedPrivatePtr priv = conn->privateData;
     const char *uuidstr = fname + strlen(LIBVIRTD_DOMAINS_DIR) + 1;
     unsigned char uuid[VIR_UUID_BUFLEN];
     int i;
@@ -208,22 +208,22 @@ xenInotifyXendDomainsDirRemoveEntry(virC
     }
 
     /* match and remove on uuid */
-    for (i=0; i<configInfoList->count; i++) {
-        if (!memcmp(uuid, configInfoList->doms[i]->uuid, VIR_UUID_BUFLEN)) {
-            VIR_FREE(configInfoList->doms[i]->name);
-            VIR_FREE(configInfoList->doms[i]);
+    for (i = 0 ; i < priv->configInfoList->count ; i++) {
+        if (!memcmp(uuid, priv->configInfoList->doms[i]->uuid, VIR_UUID_BUFLEN)) {
+            VIR_FREE(priv->configInfoList->doms[i]->name);
+            VIR_FREE(priv->configInfoList->doms[i]);
 
-            if (i < (configInfoList->count - 1))
-                memmove(configInfoList->doms + i,
-                        configInfoList->doms + i + 1,
-                        sizeof(*(configInfoList->doms)) *
-                                (configInfoList->count - (i + 1)));
+            if (i < (priv->configInfoList->count - 1))
+                memmove(priv->configInfoList->doms + i,
+                        priv->configInfoList->doms + i + 1,
+                        sizeof(*(priv->configInfoList->doms)) *
+                                (priv->configInfoList->count - (i + 1)));
 
-            if (VIR_REALLOC_N(configInfoList->doms,
-                              configInfoList->count - 1) < 0) {
+            if (VIR_REALLOC_N(priv->configInfoList->doms,
+                              priv->configInfoList->count - 1) < 0) {
                 ; /* Failure to reduce memory allocation isn't fatal */
             }
-            configInfoList->count--;
+            priv->configInfoList->count--;
             return 0;
         }
     }
@@ -235,13 +235,15 @@ xenInotifyXendDomainsDirAddEntry(virConn
                                  const char *fname) {
     char *name = NULL;
     unsigned char uuid[VIR_UUID_BUFLEN];
+    xenUnifiedPrivatePtr priv = conn->privateData;
+
     if (xenInotifyDomainLookup(conn, fname, &name, uuid) < 0) {
         virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR,
                            "%s", _("Error looking up domain"));
         return -1;
     }
 
-    if( xenUnifiedAddDomainInfo(configInfoList,
+    if (xenUnifiedAddDomainInfo(priv->configInfoList,
                                 -1, name, uuid) < 0) {
         virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR,
                         "%s", _("Error adding file to config cache"));
@@ -255,15 +257,19 @@ static int
 static int
 xenInotifyRemoveDomainConfigInfo(virConnectPtr conn,
                                  const char *fname) {
-    return useXenConfigCache ? xenXMConfigCacheRemoveFile(conn, fname) :
-                               xenInotifyXendDomainsDirRemoveEntry(conn, fname);
+    xenUnifiedPrivatePtr priv = conn->privateData;
+    return priv->useXenConfigCache ?
+        xenXMConfigCacheRemoveFile(conn, fname) :
+        xenInotifyXendDomainsDirRemoveEntry(conn, fname);
 }
 
 static int
 xenInotifyAddDomainConfigInfo(virConnectPtr conn,
                               const char *fname) {
-    return useXenConfigCache ? xenXMConfigCacheAddFile(conn, fname) :
-                               xenInotifyXendDomainsDirAddEntry(conn, fname);
+    xenUnifiedPrivatePtr priv = conn->privateData;
+    return priv->useXenConfigCache ?
+        xenXMConfigCacheAddFile(conn, fname) :
+        xenInotifyXendDomainsDirAddEntry(conn, fname);
 }
 
 static void
@@ -277,7 +283,7 @@ xenInotifyEvent(int watch ATTRIBUTE_UNUS
     struct inotify_event *e;
     int got;
     char *tmp, *name;
-    virConnectPtr conn = (virConnectPtr) data;
+    virConnectPtr conn = data;
     xenUnifiedPrivatePtr priv = NULL;
 
     DEBUG0("got inotify event");
@@ -315,7 +321,8 @@ reread:
 
         name = (char *)&(e->name);
 
-        snprintf(fname, 1024, "%s/%s", configDir, name);
+        snprintf(fname, 1024, "%s/%s",
+                 priv->configDir, name);
 
         if (e->mask & (IN_DELETE | IN_MOVED_FROM)) {
             virDomainEventPtr event =
@@ -378,24 +385,24 @@ xenInotifyOpen(virConnectPtr conn ATTRIB
 
     if(priv->xendConfigVersion <= 2) {
         /* /etc/xen */
-        configDir = xenXMGetConfigDir();
-        useXenConfigCache = 1;
+        priv->configDir = xenXMGetConfigDir();
+        priv->useXenConfigCache = 1;
     } else {
         /* /var/lib/xend/domains/<uuid>/config.sxp */
-        configDir = LIBVIRTD_DOMAINS_DIR;
-        useXenConfigCache = 0;
+        priv->configDir = LIBVIRTD_DOMAINS_DIR;
+        priv->useXenConfigCache = 0;
 
-        if ( VIR_ALLOC(configInfoList ) < 0) {
+        if (VIR_ALLOC(priv->configInfoList) < 0) {
             virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR,
-                                 "%s", _("failed to allocate configInfoList"));
+                               "%s", _("failed to allocate configInfoList"));
             return -1;
         }
 
         /* populate initial list */
-        if (!(dh = opendir(configDir))) {
+        if (!(dh = opendir(priv->configDir))) {
             virReportSystemError(NULL, errno,
                                  _("cannot open directory: %s"),
-                                 configDir);
+                                 priv->configDir);
             return -1;
         }
         while ((ent = readdir(dh))) {
@@ -403,9 +410,10 @@ xenInotifyOpen(virConnectPtr conn ATTRIB
                 continue;
 
             /* Build the full file path */
-            if ((strlen(configDir) + 1 + strlen(ent->d_name) + 1) > PATH_MAX)
+            if ((strlen(priv->configDir) + 1 +
+                 strlen(ent->d_name) + 1) > PATH_MAX)
                 continue;
-            strcpy(path, configDir);
+            strcpy(path, priv->configDir);
             strcat(path, "/");
             strcat(path, ent->d_name);
 
@@ -419,24 +427,25 @@ xenInotifyOpen(virConnectPtr conn ATTRIB
     }
 
     if ((priv->inotifyFD = inotify_init()) < 0) {
-        virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR,
-                           "%s", _("initializing inotify"));
+        virReportSystemError(NULL, errno,
+                             "%s", _("initializing inotify"));
         return -1;
     }
 
-    DEBUG("Adding a watch on %s", configDir);
+    DEBUG("Adding a watch on %s", priv->configDir);
     if (inotify_add_watch(priv->inotifyFD,
-                          configDir,
+                          priv->configDir,
                           IN_CREATE |
                           IN_CLOSE_WRITE | IN_DELETE |
                           IN_MOVED_TO | IN_MOVED_FROM) < 0) {
-        virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR,
-                           _("adding watch on %s"), _(configDir));
+        virReportSystemError(NULL, errno,
+                             _("adding watch on %s"),
+                             priv->configDir);
         return -1;
     }
 
     DEBUG0("Building initial config cache");
-    if (useXenConfigCache &&
+    if (priv->useXenConfigCache &&
         xenXMConfigCacheRefresh (conn) < 0) {
         DEBUG("Failed to enable XM config cache %s", conn->err.message);
         return -1;
@@ -464,10 +473,10 @@ int
 int
 xenInotifyClose(virConnectPtr conn)
 {
-    xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) conn->privateData;
+    xenUnifiedPrivatePtr priv = conn->privateData;
 
-    if(configInfoList)
-        xenUnifiedDomainInfoListFree(configInfoList);
+    if (priv->configInfoList)
+        xenUnifiedDomainInfoListFree(priv->configInfoList);
 
     if (priv->inotifyWatch != -1)
         virEventRemoveHandle(priv->inotifyWatch);
diff --git a/src/xen_unified.h b/src/xen_unified.h
--- a/src/xen_unified.h
+++ b/src/xen_unified.h
@@ -156,6 +156,9 @@ struct _xenUnifiedPrivate {
     /* A list of xenstore watches */
     xenStoreWatchListPtr xsWatchList;
     int xsWatch;
+    /* A list of active domain name/uuids */
+    xenUnifiedDomainInfoListPtr activeDomainList;
+
 
     /* An list of callbacks */
     virDomainEventCallbackListPtr domainEventCallbacks;
@@ -164,6 +167,10 @@ struct _xenUnifiedPrivate {
     /* The inotify fd */
     int inotifyFD;
     int inotifyWatch;
+
+    const char *configDir;
+    int  useXenConfigCache ;
+    xenUnifiedDomainInfoListPtr configInfoList;
 #endif
 };
 
diff --git a/src/xs_internal.c b/src/xs_internal.c
--- a/src/xs_internal.c
+++ b/src/xs_internal.c
@@ -46,9 +46,6 @@
 #endif
 
 #ifndef PROXY
-/* A list of active domain name/uuids */
-static xenUnifiedDomainInfoListPtr activeDomainList = NULL;
-
 static char *xenStoreDomainGetOSType(virDomainPtr domain);
 
 struct xenUnifiedDriver xenStoreDriver = {
@@ -312,7 +309,7 @@ xenStoreOpen(virConnectPtr conn,
 
 #ifndef PROXY
     /* Init activeDomainList */
-    if ( VIR_ALLOC(activeDomainList) < 0) {
+    if (VIR_ALLOC(priv->activeDomainList) < 0) {
         virXenStoreError(NULL, VIR_ERR_INTERNAL_ERROR,
                                  "%s", _("failed to allocate activeDomainList"));
         return -1;
@@ -389,7 +386,7 @@ xenStoreClose(virConnectPtr conn)
 
     xenStoreWatchListFree(priv->xsWatchList);
 #ifndef PROXY
-    xenUnifiedDomainInfoListFree(activeDomainList);
+    xenUnifiedDomainInfoListFree(priv->activeDomainList);
 #endif
     if (priv->xshandle == NULL)
         return(-1);
@@ -1188,7 +1185,7 @@ int xenStoreDomainIntroduced(virConnectP
     int *new_domids;
     int nread;
 
-    xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) opaque;
+    xenUnifiedPrivatePtr priv = opaque;
 
 retry:
     new_domain_cnt = xenStoreNumOfDomains(conn);
@@ -1207,8 +1204,8 @@ retry:
     missing = 0;
     for (i=0 ; i < new_domain_cnt ; i++) {
         found = 0;
-        for (j = 0 ; j < activeDomainList->count ; j++) {
-            if (activeDomainList->doms[j]->id == new_domids[i]) {
+        for (j = 0 ; j < priv->activeDomainList->count ; j++) {
+            if (priv->activeDomainList->doms[j]->id == new_domids[i]) {
                 found = 1;
                 break;
             }
@@ -1236,7 +1233,7 @@ retry:
                 xenUnifiedDomainEventDispatch(priv, event);
 
             /* Add to the list */
-            xenUnifiedAddDomainInfo(activeDomainList,
+            xenUnifiedAddDomainInfo(priv->activeDomainList,
                                     new_domids[i], name, uuid);
 
             VIR_FREE(name);
@@ -1265,7 +1262,7 @@ int xenStoreDomainReleased(virConnectPtr
 
     xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) opaque;
 
-    if(!activeDomainList->count) return 0;
+    if(!priv->activeDomainList->count) return 0;
 
 retry:
     new_domain_cnt = xenStoreNumOfDomains(conn);
@@ -1283,10 +1280,10 @@ retry:
     }
 
     removed = 0;
-    for (j=0 ; j < activeDomainList->count ; j++) {
+    for (j=0 ; j < priv->activeDomainList->count ; j++) {
         found = 0;
         for (i=0 ; i < new_domain_cnt ; i++) {
-            if (activeDomainList->doms[j]->id == new_domids[i]) {
+            if (priv->activeDomainList->doms[j]->id == new_domids[i]) {
                 found = 1;
                 break;
             }
@@ -1295,18 +1292,18 @@ retry:
         if (!found) {
             virDomainEventPtr event =
                 virDomainEventNew(-1,
-                                  activeDomainList->doms[j]->name,
-                                  activeDomainList->doms[j]->uuid,
+                                  priv->activeDomainList->doms[j]->name,
+                                  priv->activeDomainList->doms[j]->uuid,
                                   VIR_DOMAIN_EVENT_STOPPED,
                                   VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
             if (event)
                 xenUnifiedDomainEventDispatch(priv, event);
 
             /* Remove from the list */
-            xenUnifiedRemoveDomainInfo(activeDomainList,
-                                       activeDomainList->doms[j]->id,
-                                       activeDomainList->doms[j]->name,
-                                       activeDomainList->doms[j]->uuid);
+            xenUnifiedRemoveDomainInfo(priv->activeDomainList,
+                                       priv->activeDomainList->doms[j]->id,
+                                       priv->activeDomainList->doms[j]->name,
+                                       priv->activeDomainList->doms[j]->uuid);
 
             removed = 1;
         }

-- 
|: 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]