Re: [libvirt] [PATCHv4 2/6] virsh: add support for virConnectListAllDomains and clean up cmdList
On 06/21/12 04:34, Eric Blake wrote:
On 06/20/2012 07: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, the function vshDomainListCollect was added
s/compatiblity/compatibility/
> that tries to 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 v3:
> -Fixed compacting of domain list in fallback filter
> -removed redundant filter for active state
> -split out re-indentation and rename of namesorter to a separate patch
> -renamed functions
> -renamed flags
> -tweaked docs (typos/grammar)
> + /* 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;
> + }
Here, I think it is safe to assume that failure of
virDomainGetAutostart() implies that we can treat the domain like it
does not support autostart (that is, match DOMAINS_NO_AUTOSTART),
instead of erroring out. But up to you; I'm also okay with the error,
because in that case, you can avoid the error by not passing in the flag
to virsh in the first place.
I've decided to go with this as it's here with the option to fix this in
a later patch:
My idea is to print warnings/errors when the api call fails and continue
with the default values. This will notify the user that something has
failed, but will produce the list. Will this solution be OK?
> +
> + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_AUTOSTART) && autostart)
||
> + (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART) &&
!autostart)))
> + goto remove_entry;
> + }
> +
ACK with typo fixes and memory leak plugged. I'm okay if you push
without posting a v5, now that the fixes are down to a manageable amount.
I've fixed the leak, typos and changed the filter group matching to
macros provided in virdomainlist.h and pushed.
Thanks
Peter