[libvirt] [PATCH 03/12] libxl: Earlier detection of not running on Xen

[Daniel P. Berrange]

On Tue, Sep 03, 2013 at 05:07:25PM -0600, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> > On Fri, Aug 30, 2013 at 03:46:49PM -0600, Jim Fehlig wrote:
> >   
> >> Detect early on in libxl driver initialization if the driver
> >> should be loaded at all, avoiding needless initialization steps
> >> that only have to be undone later.  While at it, move th
> >> detection to a helper function to improve readability.
> >>
> >> After detecting that the driver should be loaded, subsequent
> >> failures such as initializing the log stream, allocating libxl
> >> ctx, etc. should be treated as failure to initialize the driver.
> >>
> >> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> >> ---
> >>  src/libxl/libxl_driver.c | 62 +++++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 40 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> >> index ff4f6be..759fed5 100644
> >> --- a/src/libxl/libxl_driver.c
> >> +++ b/src/libxl/libxl_driver.c
> >> @@ -775,34 +775,54 @@ libxlStateCleanup(void)
> >>      return 0;
> >>  }
> >>  
> >> -static int
> >> -libxlStateInitialize(bool privileged,
> >> -                     virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> >> -                     void *opaque ATTRIBUTE_UNUSED)
> >> +static bool
> >> +libxlDriverShouldLoad(bool privileged)
> >>  {
> >> -    const libxl_version_info *ver_info;
> >> -    char *log_file = NULL;
> >> +    bool ret = false;
> >>      virCommandPtr cmd;
> >> -    int status, ret = 0;
> >> -    unsigned int free_mem;
> >> -    char ebuf[1024];
> >> +    int status;
> >>  
> >> -    /* Disable libxl driver if non-root */
> >> +    /* Don't load if non-root */
> >>      if (!privileged) {
> >>          VIR_INFO("Not running privileged, disabling libxenlight driver");
> >> -        return 0;
> >> +        return ret;
> >> +    }
> >> +
> >> +    /* Don't load if not running on a Xen control domain (dom0) */
> >> +    if (!virFileExists("/proc/xen/capabilities")) {
> >> +        VIR_INFO("No Xen capabilities detected, probably not running "
> >> +                 "in a Xen Dom0.  Disabling libxenlight driver");
> >> +
> >> +        return ret;
> >>      }
> >>  
> >> -    /* Disable driver if legacy xen toolstack (xend) is in use */
> >> +   /* Don't load if legacy xen toolstack (xend) is in use */
> >>     
> >
> > Indentation typo
> >
> >   
> >>      cmd = virCommandNewArgList("/usr/sbin/xend", "status", NULL);
> >>      if (virCommandRun(cmd, &status) == 0 && status == 0) {
> >>          VIR_INFO("Legacy xen tool stack seems to be in use, disabling "
> >>                    "libxenlight driver.");
> >> -        virCommandFree(cmd);
> >> -        return 0;
> >> +    } else {
> >> +        ret = true;
> >>      }
> >>      virCommandFree(cmd);
> >>  
> >> +    return ret;
> >> +}
> >> +
> >> +static int
> >> +libxlStateInitialize(bool privileged,
> >> +                     virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> >> +                     void *opaque ATTRIBUTE_UNUSED)
> >> +{
> >> +    const libxl_version_info *ver_info;
> >> +    char *log_file = NULL;
> >> +    int ret = 0;
> >> +    unsigned int free_mem;
> >> +    char ebuf[1024];
> >> +
> >> +    if (!libxlDriverShouldLoad(privileged))
> >> +        return 0;
> >> +
> >>      if (VIR_ALLOC(libxl_driver) < 0)
> >>          return -1;
> >>  
> >> @@ -883,21 +903,20 @@ libxlStateInitialize(bool privileged,
> >>      libxl_driver->logger =
> >>              (xentoollog_logger *)xtl_createlogger_stdiostream(libxl_driver->logger_file, XTL_DEBUG, 0);
> >>      if (!libxl_driver->logger) {
> >> -        VIR_INFO("cannot create logger for libxenlight, disabling driver");
> >> -        goto fail;
> >> +        VIR_ERROR(_("Failed to create logger for libxenlight"));
> >> +        goto error;
> >>      }
> >>  
> >>      if (libxl_ctx_alloc(&libxl_driver->ctx,
> >>                         LIBXL_VERSION, 0,
> >>                         libxl_driver->logger)) {
> >> -        VIR_INFO("cannot initialize libxenlight context, probably not running "
> >> -                 "in a Xen Dom0, disabling driver");
> >> -        goto fail;
> >> +        VIR_ERROR(_("Failed to initialize libxenlight context"));
> >> +        goto error;
> >>      }
> >>  
> >>      if ((ver_info = libxl_get_version_info(libxl_driver->ctx)) == NULL) {
> >> -        VIR_INFO("cannot version information from libxenlight, disabling driver");
> >> -        goto fail;
> >> +        VIR_ERROR(_("Failed to get version information from libxenlight"));
> >>     
> >
> >
> > For the errors here, it is preferrable to use  virReportError() which will
> > in turn call VIR_ERROR anywway.
> >   
> 
> I'll change these to virReportError().  Is it generally preferable to
> use virReportError() over VIR_ERROR?  If so, I'll send a followup patch
> to replace any remaining uses of VIR_ERROR.

Yep, originally we used VIR_ERROR in anything server side which was not
directly related to an RPC call, but we stopped having that requirement
a while  back. So basically everything should use virReportError these
days and not VIR_ERROR.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|