On Fri, May 03, 2013 at 04:12:29PM -0600, Jim Fehlig wrote:
Daniel P. Berrange wrote:
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 2ecb86f..b7f1ad4 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -86,14 +86,9 @@ static struct xenUnifiedDriver const * const drivers[XEN_UNIFIED_NR_DRIVERS] = { [XEN_UNIFIED_XEND_OFFSET] = &xenDaemonDriver, [XEN_UNIFIED_XS_OFFSET] = &xenStoreDriver, [XEN_UNIFIED_XM_OFFSET] = &xenXMDriver, -#if WITH_XEN_INOTIFY - [XEN_UNIFIED_INOTIFY_OFFSET] = &xenInotifyDriver, -#endif
Looks like this was never used, so just removing it right? But there are a lot of loops in this file with 'drivers[i]->', where it might be possible that i == XEN_UNIFIED_INOTIFY_OFFSET?
The xenInotifyDriver table had one callback in it - the 'close' method. Since we directly call xenInotifyClose, we don't need this in the driver table anymore.
static int -xenUnifiedStateInitialize(bool privileged ATTRIBUTE_UNUSED, +xenUnifiedStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - inside_daemon = true; + /* Don't allow driver to work in non-root libvirtd */ + if (privileged) + inside_daemon = true;
Seems the name 'inside_daemon' is no longer appropriate. Should it be something like 'is_privileged'?
Yep, ok.
+ VIR_DEBUG("Trying XS sub-driver"); + if (xenStoreOpen(conn, auth, flags) < 0) + goto error; + VIR_DEBUG("Activated XS sub-driver"); + priv->opened[XEN_UNIFIED_XS_OFFSET] = 1;
xenNumaInit(conn);
if (!(priv->caps = xenHypervisorMakeCapabilities(conn))) { - VIR_DEBUG("Failed to make capabilities"); - goto fail; + VIR_DEBUG("Errored to make capabilities");
Maybe one too many instances of 'fail' replaced with 'error'? I think "Failed to make capabilities" is better than "Errored to make capabilities" :).
Hah, yes, search + replace gone too far.
+ goto error; }
if (!(priv->xmlopt = xenDomainXMLConfInit())) - goto fail; + goto error;
#if WITH_XEN_INOTIFY - if (xenHavePrivilege()) { - VIR_DEBUG("Trying Xen inotify sub-driver"); - if (xenInotifyOpen(conn, auth, flags) == VIR_DRV_OPEN_SUCCESS) { - VIR_DEBUG("Activated Xen inotify sub-driver"); - priv->opened[XEN_UNIFIED_INOTIFY_OFFSET] = 1; - } - } + VIR_DEBUG("Trying Xen inotify sub-driver"); + if (xenInotifyOpen(conn, auth, flags) < 0) + goto error;
The old code silently continued on if xenInotifyOpen() didn't return success.
I've audited the xenInotifyOpen method and the only reasons it would return -1 are all genuine fatal errors which we should having been honouring. Regards, 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 :|