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@xxxxxxxx>
> >> ---
> >> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list