[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 04/40] Simplify opening of Xen drivers



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]