Re: [libvirt] [PATCH] Xen Events Updated

On Thu, Nov 20, 2008 at 03:49:07PM -0500, Ben Guthro wrote: I hit a few problems with the old Xen 3.0.3 codepath for /etc/xen files in this patch. What follows is the patch I needed to make it work reliably on RHEL-5 Xen. The changes are - Remove hardcoded qemu:///system to let libvirt probe HV - Add a cast to workaround lack of const-ness in RHEL5 python - Add an explicit xenXMConfigRemoveFile() to deal with files going away - Remove use of IN_MODIFY - it fired when the config was still incompletely written resulting in wierd errors - Add use of IN_CLOSE_WRITE so we're notified when the file is finished writing - Ignoring IN_CREATE if stat() shows zero size, because this fires the moment the name is allocated in the FS, but before content is created. We can't ignore it completely, because its needed if someone creates the file via hard-linking, when the entire content appears attomically & no IN_CLOSED_WRITE is fired. - Allocate capabilities info before initializing inotify driver because loading XM config files /etc/xen requires this - Fix removal of file handles for inotify & xenstore, since we need to remove based on watch number, not FD number Regards, Daniel diff -r fe87b41b48e3 examples/domain-events/events-c/event-test.c --- a/examples/domain-events/events-c/event-test.c Fri Nov 21 08:02:15 2008 -0500 +++ b/examples/domain-events/events-c/event-test.c Fri Nov 21 08:40:58 2008 -0500 @@ -308,7 +308,7 @@ int main(int argc, char **argv) myEventRemoveTimeoutFunc); virConnectPtr dconn = NULL; - dconn = virConnectOpen (argv[1] ? argv[1] : "qemu:///system"); + dconn = virConnectOpen (argv[1] ? argv[1] : NULL); if (!dconn) { printf("error opening\n"); return -1; diff -r fe87b41b48e3 include/libvirt/virterror.h --- a/include/libvirt/virterror.h Fri Nov 21 08:02:15 2008 -0500 +++ b/include/libvirt/virterror.h Fri Nov 21 08:02:47 2008 -0500 @@ -60,6 +60,7 @@ typedef enum { VIR_FROM_DOMAIN, /* Error from domain config */ VIR_FROM_UML, /* Error at the UML driver */ VIR_FROM_NODEDEV, /* Error from node device monitor */ + VIR_FROM_XEN_INOTIFY, /* Error from xen inotify layer */ } virErrorDomain; diff -r fe87b41b48e3 python/libvir.c --- a/python/libvir.c Fri Nov 21 08:02:15 2008 -0500 +++ b/python/libvir.c Fri Nov 21 08:36:40 2008 -0500 @@ -1564,7 +1564,8 @@ getLibvirtModuleObject (void) { return libvirt_module; // PyImport_ImportModule returns a new reference - libvirt_module = PyImport_ImportModule("libvirt"); + /* Bogus (char *) cast for RHEL-5 python API brokenness */ + libvirt_module = PyImport_ImportModule((char *)"libvirt"); if(!libvirt_module) { #if DEBUG_ERROR printf("%s Error importing libvirt module\n", __FUNCTION__); diff -r fe87b41b48e3 src/util.c --- a/src/util.c Fri Nov 21 08:02:15 2008 -0500 +++ b/src/util.c Fri Nov 21 11:07:03 2008 -0500 @@ -613,6 +613,10 @@ virRun(virConnectPtr conn, VIR_FREE(outbuf); VIR_FREE(errbuf); VIR_FREE(argv_str); + if (outfd != -1) + close(outfd); + if (errfd != -1) + close(errfd); return ret; } diff -r fe87b41b48e3 src/xen_inotify.c --- a/src/xen_inotify.c Fri Nov 21 08:02:15 2008 -0500 +++ b/src/xen_inotify.c Fri Nov 21 11:07:22 2008 -0500 @@ -49,12 +49,6 @@ static const char *configDir = NU static const char *configDir = NULL; static int useXenConfigCache = 0; static xenUnifiedDomainInfoListPtr configInfoList = NULL; - -/* declared in xm_internal.c */ -virHashTablePtr xenXMGetConfigCache(void); -char *xenXMGetConfigDir(void); -int xenXMConfigCacheRefresh (virConnectPtr conn); -int xenXMConfigCacheAddRemoveFile(virConnectPtr conn, const char *filename); struct xenUnifiedDriver xenInotifyDriver = { xenInotifyOpen, /* open */ @@ -121,7 +115,7 @@ xenInotifyXendDomainsDirLookup(virConnec int i; virDomainPtr dom; const char *uuid_str; - const unsigned char uuid[VIR_UUID_BUFLEN]; + unsigned char uuid[VIR_UUID_BUFLEN]; /* xend is managing domains. we will get * a filename in the manner: @@ -145,7 +139,7 @@ xenInotifyXendDomainsDirLookup(virConnec for (i=0; i<configInfoList->count; i++) { if (STREQ(uuid_str, configInfoList->doms[i]->uuid)) { if(!(dom = virGetDomain(conn, configInfoList->doms[i]->name, - configInfoList->doms[i]->uuid))) { + (unsigned char *)configInfoList->doms[i]->uuid))) { virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR, "finding dom for %s", uuid_str); return NULL; @@ -238,14 +232,14 @@ static int static int xenInotifyRemoveDomainConfigInfo(virConnectPtr conn, const char *fname) { - return useXenConfigCache ? xenXMConfigCacheAddRemoveFile(conn, fname) : + return useXenConfigCache ? xenXMConfigCacheRemoveFile(conn, fname) : xenInotifyXendDomainsDirRemoveEntry(conn, fname); } static int xenInotifyAddDomainConfigInfo(virConnectPtr conn, const char *fname) { - return useXenConfigCache ? xenXMConfigCacheAddRemoveFile(conn, fname) : + return useXenConfigCache ? xenXMConfigCacheAddFile(conn, fname) : xenInotifyXendDomainsDirAddEntry(conn, fname); } @@ -318,7 +312,7 @@ reread: "%s", _("Error adding file to config cache")); return; } - } else if (e->mask & ( IN_MODIFY | IN_CREATE) ) { + } else if (e->mask & ( IN_CREATE | IN_CLOSE_WRITE) ) { if (xenInotifyAddDomainConfigInfo(conn, fname) < 0 ) { virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("Error adding file to config cache")); @@ -409,7 +403,8 @@ xenInotifyOpen(virConnectPtr conn ATTRIB DEBUG("Adding a watch on %s", configDir); if (inotify_add_watch(priv->inotifyFD, configDir, - IN_CREATE | IN_MODIFY | IN_DELETE) < 0) { + IN_CREATE | + IN_CLOSE_WRITE | IN_DELETE) < 0) { virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR, "adding watch on %s", _(configDir)); return -1; @@ -417,15 +412,16 @@ xenInotifyOpen(virConnectPtr conn ATTRIB DEBUG0("Building initial config cache"); if (useXenConfigCache && - xenXMConfigCacheRefresh (conn) < 0) - return -1; - + xenXMConfigCacheRefresh (conn) < 0) { + DEBUG("Failed to enable XM config cache %s", conn->err.message); + return -1; + } + + DEBUG0("Registering with event loop"); /* Add the handle for monitoring */ - if (virEventAddHandle(priv->inotifyFD, VIR_EVENT_HANDLE_READABLE, - xenInotifyEvent, conn, NULL) < 0) { - virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to add inotify event handle")); - return -1; + if ((priv->inotifyWatch = virEventAddHandle(priv->inotifyFD, VIR_EVENT_HANDLE_READABLE, + xenInotifyEvent, conn, NULL)) < 0) { + DEBUG0("Failed to add inotify handle, disabling events"); } conn->refs++; @@ -448,7 +444,8 @@ xenInotifyClose(virConnectPtr conn) if(configInfoList) xenUnifiedDomainInfoListFree(configInfoList); - virEventRemoveHandle(priv->inotifyFD); + if (priv->inotifyWatch != -1) + virEventRemoveHandle(priv->inotifyWatch); close(priv->inotifyFD); virUnrefConnect(conn); diff -r fe87b41b48e3 src/xen_unified.c --- a/src/xen_unified.c Fri Nov 21 08:02:15 2008 -0500 +++ b/src/xen_unified.c Fri Nov 21 09:25:22 2008 -0500 @@ -291,6 +291,11 @@ xenUnifiedOpen (virConnectPtr conn, virC DEBUG0("Activated hypervisor sub-driver"); priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET] = 1; } + } + + if (!(priv->caps = xenHypervisorMakeCapabilities(conn))) { + DEBUG0("Failed to make capabilities"); + goto fail; } /* XenD is required to suceed if root. @@ -351,11 +356,6 @@ xenUnifiedOpen (virConnectPtr conn, virC } #endif - if (!(priv->caps = xenHypervisorMakeCapabilities(conn))) { - DEBUG0("Failed to make capabilities"); - goto fail; - } - return VIR_DRV_OPEN_SUCCESS; fail: @@ -1324,6 +1324,11 @@ xenUnifiedDomainEventRegister (virConnec void (*freefunc)(void *)) { GET_PRIVATE (conn); + if (priv->xsWatch == -1) { + xenUnifiedError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + return -1; + } + conn->refs++; return virDomainEventCallbackListAdd(conn, priv->domainEventCallbacks, callback, opaque, freefunc); @@ -1335,6 +1340,11 @@ xenUnifiedDomainEventDeregister (virConn { int ret; GET_PRIVATE (conn); + if (priv->xsWatch == -1) { + xenUnifiedError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + return -1; + } + ret = virDomainEventCallbackListRemove(conn, priv->domainEventCallbacks, callback); virUnrefConnect(conn); diff -r fe87b41b48e3 src/xen_unified.h --- a/src/xen_unified.h Fri Nov 21 08:02:15 2008 -0500 +++ b/src/xen_unified.h Fri Nov 21 09:16:19 2008 -0500 @@ -155,6 +155,7 @@ struct _xenUnifiedPrivate { /* A list of xenstore watches */ xenStoreWatchListPtr xsWatchList; + int xsWatch; /* An list of callbacks */ virDomainEventCallbackListPtr domainEventCallbacks; @@ -162,6 +163,7 @@ struct _xenUnifiedPrivate { #if WITH_XEN_INOTIFY /* The inotify fd */ int inotifyFD; + int inotifyWatch; #endif }; diff -r fe87b41b48e3 src/xm_internal.c --- a/src/xm_internal.c Fri Nov 21 08:02:15 2008 -0500 +++ b/src/xm_internal.c Fri Nov 21 10:51:01 2008 -0500 @@ -45,6 +45,8 @@ #include "uuid.h" #include "util.h" #include "memory.h" +#include "logging.h" + /* The true Xen limit varies but so far is always way less than 1024, which is the Linux kernel limit according @@ -65,10 +67,6 @@ char * xenXMAutoAssignMac(void); char * xenXMAutoAssignMac(void); static int xenXMDomainAttachDevice(virDomainPtr domain, const char *xml); static int xenXMDomainDetachDevice(virDomainPtr domain, const char *xml); -virHashTablePtr xenXMGetConfigCache (void); -char *xenXMGetConfigDir (void); -int xenXMConfigCacheRefresh (virConnectPtr conn); -int xenXMConfigCacheAddRemoveFile(virConnectPtr conn, const char *filename); #define XM_REFRESH_INTERVAL 10 @@ -377,15 +375,45 @@ xenXMConfigSaveFile(virConnectPtr conn, } int -xenXMConfigCacheAddRemoveFile(virConnectPtr conn, const char *filename) +xenXMConfigCacheRemoveFile(virConnectPtr conn ATTRIBUTE_UNUSED, + const char *filename) +{ + xenXMConfCachePtr entry; + + entry = virHashLookup(configCache, filename); + if (!entry) { + DEBUG("No config entry for %s", filename); + return 0; + } + + virHashRemoveEntry(nameConfigMap, entry->def->name, NULL); + virHashRemoveEntry(configCache, filename, xenXMConfigFree); + DEBUG("Removed %s %s", entry->def->name, filename); + return 0; +} + + +int +xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename) { xenXMConfCachePtr entry; struct stat st; int newborn = 0; time_t now = time(NULL); + DEBUG("Adding file %s", filename); + /* Get modified time */ if ((stat(filename, &st) < 0)) { + xenXMError (conn, VIR_ERR_INTERNAL_ERROR, + "cannot stat %s: %s", filename, strerror(errno)); + return -1; + } + + /* Ignore zero length files, because inotify fires before + any content has actually been created */ + if (st.st_size == 0) { + DEBUG("Ignoring zero length file %s", filename); return -1; } @@ -421,6 +449,7 @@ xenXMConfigCacheAddRemoveFile(virConnect entry->refreshedAt = now; if (!(entry->def = xenXMConfigReadFile(conn, entry->filename))) { + DEBUG("Failed to read %s", entry->filename); if (!newborn) virHashRemoveEntry(configCache, filename, NULL); VIR_FREE(entry); @@ -449,6 +478,7 @@ xenXMConfigCacheAddRemoveFile(virConnect VIR_FREE(entry); } } + DEBUG("Added config %s %s", entry->def->name, filename); return 0; } @@ -526,8 +556,8 @@ int xenXMConfigCacheRefresh (virConnectP /* If we already have a matching entry and it is not modified, then carry on to next one*/ - if (xenXMConfigCacheAddRemoveFile(conn, path) < 0) { - goto cleanup; + if (xenXMConfigCacheAddFile(conn, path) < 0) { + /* Ignoring errors, since alot of stuff goes wrong in /etc/xen */ } } @@ -538,7 +568,6 @@ int xenXMConfigCacheRefresh (virConnectP virHashRemoveSet(configCache, xenXMConfigReaper, xenXMConfigFree, (const void*) &now); ret = 0; - cleanup: if (dh) closedir(dh); diff -r fe87b41b48e3 src/xm_internal.h --- a/src/xm_internal.h Fri Nov 21 08:02:15 2008 -0500 +++ b/src/xm_internal.h Fri Nov 21 10:32:03 2008 -0500 @@ -32,6 +32,12 @@ extern struct xenUnifiedDriver xenXMDriv extern struct xenUnifiedDriver xenXMDriver; int xenXMInit (void); +virHashTablePtr xenXMGetConfigCache(void); +char *xenXMGetConfigDir(void); +int xenXMConfigCacheRefresh (virConnectPtr conn); +int xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename); +int xenXMConfigCacheRemoveFile(virConnectPtr conn, const char *filename); + int xenXMOpen(virConnectPtr conn, virConnectAuthPtr auth, int flags); int xenXMClose(virConnectPtr conn); const char *xenXMGetType(virConnectPtr conn); diff -r fe87b41b48e3 src/xs_internal.c --- a/src/xs_internal.c Fri Nov 21 08:02:15 2008 -0500 +++ b/src/xs_internal.c Fri Nov 21 09:25:57 2008 -0500 @@ -44,10 +44,10 @@ #error "unsupported platform" #endif +#ifndef PROXY /* A list of active domain name/uuids */ static xenUnifiedDomainInfoListPtr activeDomainList = NULL; -#ifndef PROXY static char *xenStoreDomainGetOSType(virDomainPtr domain); struct xenUnifiedDriver xenStoreDriver = { @@ -345,16 +345,12 @@ xenStoreOpen(virConnectPtr conn, } /* Add an event handle */ - if (virEventAddHandle(xs_fileno(priv->xshandle), - VIR_EVENT_HANDLE_READABLE, - xenStoreWatchEvent, - conn, - NULL) < 0) { - virXenStoreError(NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to handle")); - - return -1; - } + if ((priv->xsWatch = virEventAddHandle(xs_fileno(priv->xshandle), + VIR_EVENT_HANDLE_READABLE, + xenStoreWatchEvent, + conn, + NULL)) < 0) + DEBUG0("Failed to add event handle, disabling events\n"); #endif //PROXY return 0; @@ -371,7 +367,6 @@ int int xenStoreClose(virConnectPtr conn) { - int fd; xenUnifiedPrivatePtr priv; if (conn == NULL) { @@ -398,9 +393,8 @@ xenStoreClose(virConnectPtr conn) if (priv->xshandle == NULL) return(-1); - fd = xs_fileno(priv->xshandle); - virEventRemoveHandle(fd); - close(fd); + if (priv->xsWatch != -1) + virEventRemoveHandle(priv->xsWatch); xs_daemon_close(priv->xshandle); priv->xshandle = NULL; -- |: 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 :|