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

Re: [libvirt] [PATCH] Xen Events Updated



I tested these changes, and seem to be having some issues opening the xen driver in a post 3.0.3 codepath:

DEBUG: xen_unified.c: xenUnifiedOpen (Trying hypervisor sub-driver)
DEBUG: xen_unified.c: xenUnifiedOpen (Activated hypervisor sub-driver)
libvir: Xen Daemon error : internal error failed to create a socket   
libvir: Xen error : out of memory                                     
DEBUG: xen_unified.c: xenUnifiedOpen (Failed to make capabilities)    
DEBUG: xen_unified.c: xenUnifiedOpen (Failed to activate a mandatory sub-driver)
DEBUG: libvirt.c: do_open (driver 1 Xen returned ERROR)


I haven't chased this down fully yet, but something is interfering...



Daniel P. Berrange wrote on 11/21/2008 11:13 AM:
> On Thu, Nov 20, 2008 at 03:49:07PM -0500, Ben Guthro wrote:
>> New xen events patch attached. This removes a couple unnecessary 
>> changes from my prior patch, but remains functionally the same as
>> the last version.
>>
>> This will emit the following events for Xen:
>>
>> STARTED
>> STOPPED
>> ADDED
>> REMOVED
> 
> 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;
>  
> 
> 
> 


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