[libvirt] [PATCH 08/40] Simplify the Xen count/list domains driver methods

[Daniel P. Berrange]

On Mon, May 06, 2013 at 02:32:13PM -0600, Jim Fehlig wrote:
> Jim Fehlig wrote:
> > Daniel P. Berrange wrote:
> >   
> >> From: "Daniel P. Berrange" <berrange at redhat.com>
> >>
> >> The XenStore driver is mandatory, so it can be used unconditonally
> >> for the xenUnifiedConnectListDomains & xenUnifiedConnectNumOfDomains
> >> drivers. Delete the unused XenD and Hypervisor driver code for
> >> listing / counting domains
> >>
> >> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> >> ---
> >>  src/xen/xen_driver.c     |  46 +--------------------
> >>  src/xen/xen_hypervisor.c | 101 -----------------------------------------------
> >>  src/xen/xen_hypervisor.h |   4 --
> >>  src/xen/xend_internal.c  |  81 -------------------------------------
> >>  src/xen/xend_internal.h  |   2 -
> >>  src/xen/xs_internal.c    |   8 ----
> >>  6 files changed, 2 insertions(+), 240 deletions(-)
> >>
> >> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
> >> index b6cf6ca..25fb7bb 100644
> >> --- a/src/xen/xen_driver.c
> >> +++ b/src/xen/xen_driver.c
> >> @@ -583,55 +583,13 @@ xenUnifiedConnectGetCapabilities(virConnectPtr conn)
> >>  static int
> >>  xenUnifiedConnectListDomains(virConnectPtr conn, int *ids, int maxids)
> >>  {
> >> -    xenUnifiedPrivatePtr priv = conn->privateData;
> >> -    int ret;
> >> -
> >> -    /* Try xenstore. */
> >> -    if (priv->opened[XEN_UNIFIED_XS_OFFSET]) {
> >> -        ret = xenStoreListDomains(conn, ids, maxids);
> >> -        if (ret >= 0) return ret;
> >> -    }
> >> -
> >> -    /* Try HV. */
> >> -    if (priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET]) {
> >> -        ret = xenHypervisorListDomains(conn, ids, maxids);
> >> -        if (ret >= 0) return ret;
> >> -    }
> >> -
> >> -    /* Try xend. */
> >> -    if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) {
> >> -        ret = xenDaemonListDomains(conn, ids, maxids);
> >> -        if (ret >= 0) return ret;
> >> -    }
> >> -
> >> -    return -1;
> >> +    return xenStoreListDomains(conn, ids, maxids);
> >>   
> >>     
> >
> > Probably not likely, but what if xenStoreListDomains() failed, e.g.
> > xshandle somehow became stale? The existing code would try the other
> > drivers if xenstore one failed.
> >   
> 
> I started to make a similar comment for patch 10, but then re-read your
> cover letter and realize this is intentional. While I agree with the
> direct invocation in 10, and probably others I've yet to review, this
> may be a case where we actually want to try something other than
> xenstore. I seem to recall an issue long ago where trying multiple
> drivers helped when there were state inconsistencies in the xen tools.
> But then again, maybe it is best to simply fail instead of propagating
> those inconsistencies?

The issue is that trying multiple drivers doesn't actually solve any
inconsistencies - the first driver to succeed wins, even if it has
stale data. You are right though that we did have some issues in the
past - this is why with certain methods we twiddle it so that it would
try XenStore before trying XenD, since XenStore was faster & more
accurate. This cleanup as maintained this prioritization, so I don't
think we're going to regress in this respect.

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