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

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



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



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