[libvirt] [PATCHv3 02/12] virsh: add support for virConnectListAllDomains and clean up cmdList
Eric Blake
eblake at redhat.com
Mon Jun 11 20:16:18 UTC 2012
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.
> @@ -490,22 +491,307 @@ _vshStrdup(vshControl *ctl, const char *s, const char *filename, int line)
> #define realloc use_vshRealloc_instead_of_realloc
> #define strdup use_vshStrdup_instead_of_strdup
>
> -static int idsorter(const void *a, const void *b) {
> - const int *ia = (const int *)a;
> - const int *ib = (const int *)b;
> +static int
> +namesorter(const void *a, const void *b)
This is code motion, but the name isn't very namespace friendly. It
might be worth a followup patch that renames this vshNameSorter and
adjusts all clients. Splitting code motion into separate patches from
new code makes both patches easier to review.
> +
> +static int
> +domsorter(const void *a, const void *b)
Likewise, I would name this vshDomSorter.
> +#define MATCH(FLAG) (flags & (FLAG))
> +static int
> +vshDomList(vshControl *ctl, vshDomainListPtr domlist, unsigned int flags)
In my patch proposal, I had the list collected as the result, as in:
static vshDomainListPtr
vshDomListCollect(vshControl *ctl, unsigned int flags)
rather than making the caller pre-allocate vshDomainListPtr.
> +
> + /* try the list with flags support */
In my snapshot patches, I tried to mention a bit more in the comments,
such as 0.9.13 or newer...
> + if ((ndoms = virConnectListAllDomains(ctl->conn, &doms, flags)) >= 0) {
> + domlist->ndomains = ndoms;
> + domlist->domains = doms;
Would it make any more sense to just directly import into
domlist->domains instead of going through a temporary doms?
> + doms = NULL;
> + goto finished;
> + }
> +
> + /* check if the command is actualy supported */
s/actualy/actually/
> + if ((err = virGetLastError()) &&
> + err->code == VIR_ERR_NO_SUPPORT)
> + goto fallback;
> +
> + if ((err= virGetLastError()) &&
Missing space before =
> + err->code == VIR_ERR_INVALID_ARG) {
and double space after ==
> + /* try again the new API but without flags this time */
grammar:
try the new API again, but without flags this time
> + if ((ndoms = virConnectListAllDomains(ctl->conn, &doms, 0)) >= 0)
This misses _all_ flags, but the 'filter:' label assumes that the
_ACTIVE/_INACTIVE flag was already honored. Again, if we promise in the
API call that the _ACTIVE/_INACTIVE group is always supported, then here
you could pass 'flags & (_ACTIVE|_INACTIVE)' instead of 0, so that
'filter' is operating from the same state.
> + goto filter;
> + }
> +
> + /* there was an error during the first or second call */
> + vshError(ctl, "%s", _("Failed to list domains"));
> + goto cleanup;
> +
> +
> +fallback:
> + /* fall back to old method */
...mainly 0.9.12 and earlier.
> +
> + /* get active domains */
> + for (i = 0; i < nids; i++) {
> + if (!(dom = virDomainLookupByID(ctl->conn, ids[i])))
> + continue;
> + doms[ndoms++] = dom;
> + }
> +
> + /* get inctive domains */
s/inctive/inactive/
> + for (i = 0; i < nnames; i++) {
> + if (!(dom = virDomainLookupByName(ctl->conn, names[i])))
> + continue;
> + doms[ndoms++] = dom;
Again, instead of going through a temporary doms, would it be worth
directly sticking the virDomainLookupByName() result into domlist->domains?
> + }
> +
> +filter:
> + /* filter list the list if the list was acquired by fallback means */
> + for (i = 0; i < ndoms; i++) {
> + dom = doms[i];
> +
> + /* active state filter */
> + if (MATCH(VIR_CONNECT_LIST_DOMAINS_ACTIVE |
> + VIR_CONNECT_LIST_DOMAINS_INACTIVE)) {
I'm not sure you need to repeat this - if we guarantee that the new API
always supports _ACTIVE/_INACTIVE, and the older API already guarantees
it by virtue of the split API call, then this filter has already been
applied by the time we get here.
> + /* autostart filter */
> + if (MATCH(VIR_CONNECT_LIST_DOMAINS_AUTOSTART |
> + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART)) {
> + if (virDomainGetAutostart(dom, &autostart) < 0) {
> + vshError(ctl, "%s", _("Failed to get domain autostart state"));
> + goto cleanup;
> + }
> +
> + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_AUTOSTART) && autostart) ||
> + (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART) && autostart)))
Missing '!' before the second 'autostart'
> +
> + /* we're shortening the array, so it's safe to ignore value */
> + ignore_value(VIR_REALLOC_N(doms, count));
I'm not sure if VIR_SHRINK_N does any better here, but what you have works.
> +
> + domlist->domains = doms;
> + domlist->ndomains = count;
> + doms = NULL;
> +
> +finished:
> + /* sort the list */
> + if (domlist->domains && domlist->ndomains > 0)
> + qsort(domlist->domains, domlist->ndomains,
> + sizeof(*domlist->domains), domsorter);
In my snapshot patch series, I left the NULL entries in place, then
taught the sorter to sink NULL entries to the end of the array, and did
the sort before shrinking the size of the array instead of reassigning.
And in fact, I think you have a bug in your approach, because you are
packing things up front without clearing out the old location. Consider:
- you reach the filter: label with 3 elements: [dom0], [dom1], [dom2]
- dom0 fails the first filter, so you clean it up right away.
- dom1 passes, and you copy it to the front of the array
- something goes wrong, and you hit an error while querying whether dom2
needs to be filtered
- now you reach the cleanup: label with [dom1], [dom1], [dom2]; and you
now end up calling virDomainFree(dom1) twice. Oops.
I think your approach of repositioning would be safe _if_ you also set
the old position to NULL at the time you moved things (but be careful
not to set the old position to NULL if the old and new position are
identical); but if you don't try to pack the array during the filtering,
but instead let the qsort() do the packing, then you don't even have to
worry about the partial case that I listed above.
> +
> +To filter the list of domains present on the hypervisor you may specify one or
> +more of filtering flags supported by the B<list> command. These flags are
> +grouped by function. Specifying one or more of flags from a group enables the
s/of flags/flags/
> +=item B<Domain state>
> +
> +Following filter flags to select domains by its state are available:
s/Following/The following/
s/to select domains/select a domain/
> +I<--state-running> for runnign domains, I<--state-paused> for paused domains,
s/runnign/running/
> +I<--state-shutoff> for turned off domains and I<--state-other> for all
> +other states as a fallback.
> +
> +=item B<Autostarting domains>
> +
> +To list autostarting domains use the flag I<--autostart-yes>. To list domains
> +with this feature disabled use I<--autostart-no>.
> +
> +=item B<Snapshot existence>
> +
> +Domains that have snapshot images can be listed using flagI<--snapshot-yes>,
s/flagI</flag I</
> +
> +First attempt to acquire the domain list is done by the non-racy listing API.
> +If this API is not available for the current connection the list is constructed
> +using fallback code that might miss domains transitioning states when the
> +fallback code is executed.
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?
--
Eric Blake eblake at redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120611/a9c4c7cd/attachment-0001.sig>
More information about the libvir-list
mailing list