[libvirt] [PATCHv3 02/12] virsh: add support for virConnectListAllDomains and clean up cmdList

[Peter Krempa]

On 06/11/12 22:16, Eric Blake wrote:
> On 06/11/2012 04:33 AM, Peter Krempa wrote:
>>
>> This patch makes use of the newly added api virConnectListAllDomains()
>> to list domains in virsh.
>>
>> Virsh now represents lists of domains using an internal structure
>> vshDomainList. This structure contains the virDomainPtr list as provided
>> by virConnectListAllDomains() and the count of domains in the list.
>>
>> For backwards compatiblity function vshDomList was added that tries to
>
> s/compatiblity/compatibility, the/
>
>> enumerate the domains using the new API and if the API is not supported
>> falls back to the older approach with the two list functions. The helper
>> function also simulates filtering by all currently supported flags added
>> with virConnectListAllDomains().
>>
>> This patch also cleans up the "list" command handler to use the new
>> helpers and adds new command line flags to make use of filtering.
>> ---
>> Diff to v2:
>> - moved this patch right after API implementation to make testing fallback easier
>> - added support for all filtering flags both in fallback code and in "list" command
>> - fixed messed up syntax check silencing :)
>> - fixed spelling errors but probably introduced a ton of new as I've added new docs
>> ---
>>   tools/virsh.c   |  555 ++++++++++++++++++++++++++++++++++++++-----------------
>>   tools/virsh.pod |   91 +++++++---
>>   2 files changed, 449 insertions(+), 197 deletions(-)
>
> This turned into a rather large patch; I'm not sure if it is worth
> trying to subdivide it at all, but in the interest of getting this in
> sooner rather than later, I'll go ahead and review it as-is.
>

>
> I'm not sure if VIR_SHRINK_N does any better here, but what you have works.

Well, VIR_SHRINK_N takes number of elements to remove from the end but 
VIR_REALLOC_N takes the desired new size, so it was more intuitive to 
use REALLOC.  but VIR_SHRINK_N(doms, ndoms, ndoms-count) looks okay too.

>
>> +
>> +    domlist->domains = doms;
>> +    domlist->ndomains = count;
>> +    doms = NULL;
>> +
>
> I'd word this paragraph as:
>
> When talking to older servers, this command is forced to use a series of
> API calls with an inherent race, where a domain might not be listed or
> might appear more than once if it changed state between calls while the
> list was being collected.  Newer servers do not have this problem.
>
> Getting closer.  Do you need me to post my suggested improvements as a
> v4, or are you comfortable respinning this one?

I'll do the respin. Thanks for the suggestions.

Peter