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

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(a)suse.com&gt; >> --- >> 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.