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