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

[Jim Fehlig]

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?

Regards,
Jim