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