[libvirt] [PATCHv2 0/9] API to list all domains

Second version of the atomic listing API. This version includes fixes and enhancements after Eric's review: - Enhanced documentation - NULL element at the end of the list - filtering flags and filter support - helper function to ease implementation across drivers - implementation to most of the drivers - enhancement of the virsh helper that now supports filter simulation For some of the drivers I don't have means to test the implementation apart from compiling it. Please beaware of those. Peter Krempa (9): lib: Add public api to enable atomic listing of guest python: add API exports for virConnectListAllDomains() remote: implement remote protocol for virConnectListAllDomains() conf: Add helper for listing domains on drivers supporting virDomainObj drivers: Implement virListAllDomains for drivers using virDomainObj vbox: Add support for virDomainList hyperv: Add implementation for virConnectListAllDomains() esx: Add implementation for virConnectListAllDomains() virsh: add support for virConnectListAllDomains and clean up cmdList daemon/remote.c | 52 ++++ include/libvirt/libvirt.h.in | 36 +++- python/generator.py | 1 + python/libvirt-override-api.xml | 12 +- python/libvirt-override-virConnect.py | 12 + python/libvirt-override.c | 47 ++++- src/Makefile.am | 6 +- src/conf/virdomainlist.c | 214 +++++++++++++++ src/conf/virdomainlist.h | 36 +++ src/driver.h | 11 +- src/esx/esx_driver.c | 108 ++++++++ src/hyperv/hyperv_driver.c | 95 +++++++ src/libvirt.c | 122 +++++++++- src/libvirt_private.syms | 5 + src/libvirt_public.syms | 5 + src/libxl/libxl_driver.c | 33 +++ src/lxc/lxc_driver.c | 32 +++ src/openvz/openvz_driver.c | 30 ++ src/qemu/qemu_driver.c | 87 ++++++- src/remote/remote_driver.c | 63 +++++ src/remote/remote_protocol.x | 14 +- src/remote_protocol-structs | 12 + src/test/test_driver.c | 31 +++ src/uml/uml_driver.c | 31 +++ src/vbox/vbox_tmpl.c | 133 ++++++++++ src/vmware/vmware_driver.c | 31 +++ tools/virsh.c | 469 +++++++++++++++++++++------------ 27 files changed, 1536 insertions(+), 192 deletions(-) create mode 100644 src/conf/virdomainlist.c create mode 100644 src/conf/virdomainlist.h -- 1.7.3.4

This patch adds a new public api that lists domains. The new approach is different from those used before. There are key points to this: 1) The list is acquired atomically and contains both active and inactive domains (guests). This eliminates the need to call two different list APIs, where the state might change in between of the calls. 2) The returned list consists of virDomainPtrs instead of names or ID's that have to be converted to virDomainPtrs anyways using separate calls for each one of them. This is more convenient and saves hypervisor calls. 3) The returned list is auto-allocated. This saves a lot hassle for the users. 4) Built in support for filtering. The API call supports various filtering flags that modify the output list according to user needs. Available filter groups: Domain status: VIR_CONNECT_LIST_DOMAINS_ACTIVE, VIR_CONNECT_LIST_DOMAINS_INACTIVE Domain persistence: VIR_CONNECT_LIST_DOMAINS_PERSISTENT, VIR_CONNECT_LIST_DOMAINS_TRANSIENT Domain state: VIR_CONNECT_LIST_DOMAINS_RUNNING, VIR_CONNECT_LIST_DOMAINS_PAUSED, VIR_CONNECT_LIST_DOMAINS_SHUTOFF, VIR_CONNECT_LIST_DOMAINS_OTHER Presense of managed save image: VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE, VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE Auto-start option: VIR_CONNECT_LIST_DOMAINS_AUTOSTART, VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART Presense of snapshot: VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT, VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT 5) The python binding returns a list of domain objects that is very neat to work with. The only problem with this approach is no support from code generators so both RPC code and python bindings had to be written manualy. *include/libvirt/libvirt.h.in: - add API prototype - clean up whitespace mistakes nearby *python/generator.py: - inhibit generation of the bindings for the new api *src/driver.h: - add driver prototype - clean up some whitespace mistakes nearby *src/libvirt.c: - add public implementation *src/libvirt_public.syms: - export the new symbol --- Diff to v1: - added docs and filtering flags --- include/libvirt/libvirt.h.in | 36 ++++++++++++- python/generator.py | 1 + src/driver.h | 11 +++- src/libvirt.c | 122 +++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 5 ++ 5 files changed, 168 insertions(+), 7 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index da3ce29..230c17f 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1759,8 +1759,40 @@ int virDomainUndefineFlags (virDomainPtr domain, unsigned int flags); int virConnectNumOfDefinedDomains (virConnectPtr conn); int virConnectListDefinedDomains (virConnectPtr conn, - char **const names, - int maxnames); + char **const names, + int maxnames); +/** + * virConnectListAllDomainsFlags: + * + * Flags used to tune which domains are listed by virConnectListAllDomains(). + * Note that these flags come in groups; if all bits from a group are 0, + * then that group is not used to filter results. + */ +typedef enum { + VIR_CONNECT_LIST_DOMAINS_ACTIVE = 1 << 0, + VIR_CONNECT_LIST_DOMAINS_INACTIVE = 1 << 1, + + VIR_CONNECT_LIST_DOMAINS_PERSISTENT = 1 << 2, + VIR_CONNECT_LIST_DOMAINS_TRANSIENT = 1 << 3, + + VIR_CONNECT_LIST_DOMAINS_RUNNING = 1 << 4, + VIR_CONNECT_LIST_DOMAINS_PAUSED = 1 << 5, + VIR_CONNECT_LIST_DOMAINS_SHUTOFF = 1 << 6, + VIR_CONNECT_LIST_DOMAINS_OTHER = 1 << 7, + + VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE = 1 << 8, + VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE = 1 << 9, + + VIR_CONNECT_LIST_DOMAINS_AUTOSTART = 1 << 10, + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART = 1 << 11, + + VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT = 1 << 12, + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT = 1 << 13, +} virConnectListAllDomainsFlags; + +int virConnectListAllDomains (virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags); int virDomainCreate (virDomainPtr domain); int virDomainCreateWithFlags (virDomainPtr domain, unsigned int flags); diff --git a/python/generator.py b/python/generator.py index 9530867..0b6ac7c 100755 --- a/python/generator.py +++ b/python/generator.py @@ -453,6 +453,7 @@ skip_function = ( 'virConnectDomainEventDeregisterAny', # overridden in virConnect.py 'virSaveLastError', # We have our own python error wrapper 'virFreeError', # Only needed if we use virSaveLastError + 'virConnectListAllDomains', #overridden in virConnect.py 'virStreamRecvAll', # Pure python libvirt-override-virStream.py 'virStreamSendAll', # Pure python libvirt-override-virStream.py diff --git a/src/driver.h b/src/driver.h index aa7a377..c4e558f 100644 --- a/src/driver.h +++ b/src/driver.h @@ -251,9 +251,13 @@ typedef char * const char *domainXml, unsigned int flags); typedef int - (*virDrvListDefinedDomains) (virConnectPtr conn, - char **const names, - int maxnames); + (*virDrvListDefinedDomains) (virConnectPtr conn, + char **const names, + int maxnames); +typedef int + (*virDrvListAllDomains) (virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags); typedef int (*virDrvNumOfDefinedDomains) (virConnectPtr conn); typedef int @@ -866,6 +870,7 @@ struct _virDriver { virDrvGetCapabilities getCapabilities; virDrvListDomains listDomains; virDrvNumOfDomains numOfDomains; + virDrvListAllDomains listAllDomains; virDrvDomainCreateXML domainCreateXML; virDrvDomainLookupByID domainLookupByID; virDrvDomainLookupByUUID domainLookupByUUID; diff --git a/src/libvirt.c b/src/libvirt.c index ec8307e..431b9a2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1780,7 +1780,14 @@ error: * * Collect the list of active domains, and store their IDs in array @ids * - * Returns the number of domains found or -1 in case of error + * For inactive domains, see virConnectListDefinedDomains(). For more + * control over the results, see virConnectListAllDomains(). + * + * Returns the number of domains found or -1 in case of error. Note that + * this command is inherently racy; a domain can be started between a + * call to virConnectNumOfDomains() and this call; you are only guaranteed + * that all currently active domains were listed if the return is less + * than @maxids. */ int virConnectListDomains(virConnectPtr conn, int *ids, int maxids) @@ -7951,7 +7958,14 @@ error: * list the defined but inactive domains, stores the pointers to the names * in @names * - * Returns the number of names provided in the array or -1 in case of error + * For active domains, see virConnectListDomains(). For more control over + * the results, see virConnectListAllDomains(). + * + * Returns the number of names provided in the array or -1 in case of error. + * Note that this command is inherently racy; a domain can be defined between + * a call to virConnectNumOfDefinedDomains() and this call; you are only + * guaranteed that all currently defined domains were listed if the return + * is less than @maxids. The client must call free() on each returned name. */ int virConnectListDefinedDomains(virConnectPtr conn, char **const names, @@ -7985,6 +7999,110 @@ error: } /** + * virConnectListAllDomains: + * @conn: Pointer to the hypervisor connection. + * @domains: Pointer to a variable to store the array containing domain objects + * or NULL if the list is not required (just returns number of guests). + * @flags: bitwise-OR of virConnectListAllDomainsFlags + * + * Collect a possibly-filtered list of all domains, and return an allocated + * array of information for each. This API solves the race inherent in + * virConnectListDomains() and virConnectListDefinedDomains(). + * + * Normally, all domains are returned; however, @flags can be used to + * filter the results for a smaller list of targetted domains. The valid + * flags are divided into groups, where each group contains bits that + * describe mutually exclusive attributes of a domain, and where all bits + * within a group describe all possible domains. Some hypervisors might + * reject explicit bits from a group where the hypervisor cannot make a + * distinction (for example, not all hypervisors can tell whether domains + * have snapshots). For a group supported by a given hypervisor, the + * behavior when no bits of a group are set is identical to the behavior + * when all bits in that group are set. When setting bits from more than + * one group, it is possible to select an impossible combination (such + * as an inactive transient domain), in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_CONNECT_LIST_DOMAINS_ACTIVE (online + * domains) and VIR_CONNECT_LIST_DOMAINS_INACTIVE (offline domains). + * + * The next group of @flags is VIR_CONNECT_LIST_DOMAINS_PERSISTENT (defined + * domains) and VIR_CONNECT_LIST_DOMAINS_TRANSIENT (running but not defined). + * + * The next group of @flags covers various domain states: + * VIR_CONNECT_LIST_DOMAINS_RUNNING, VIR_CONNECT_LIST_DOMAINS_PAUSED, + * VIR_CONNECT_LIST_DOMAINS_SHUTOFF, and a catch-all for all other states + * (such as crashed, this catch-all covers the possibility of adding new + * states). + * + * The remaining groups cover boolean attributes commonly asked about + * domains; they include VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE and + * VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE, for filtering based on whether + * a managed save image exists; VIR_CONNECT_LIST_DOMAINS_AUTOSTART and + * VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART, for filtering based on autostart; + * VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT and + * VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, for filtering based on whether + * a domain has snapshots. + * + * Returns the number of domains found or -1 in case of error. On success, + * the array stored into @doms is guaranteed to have an extra allocated + * element set to NULL, to make iteration easier. The caller is + * responsible for calling virDomainFree() on each array element, then + * calling free() on @doms. + * + * Example of usage: + * virDomainPtr *domains; + * virDomainPtr dom; + * int i; + * + * int ret; + * unsigned int flags = VIR_CONNECT_LIST_RUNNING | + * VIR_CONNECT_LIST_PERSISTENT; + * + * ret = virConnectListAllDomains(conn, &domains, flags); + * if (ret < 0) + * error(); + * + * for (i = 0; i < ret; i++) { + * do_someting_with_domain(domains[i]); + * + * //here or in a separate loop if needed + * virDomainFree(domains[i]); + * } + * + * free(domains); + */ +int +virConnectListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, domains=%p, flags=%x", conn, domains, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (conn->driver->listAllDomains) { + int ret; + ret = conn->driver->listAllDomains(conn, domains, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + +/** * virDomainCreate: * @domain: pointer to a defined domain * diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 46c13fb..6fd4843 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -534,4 +534,9 @@ LIBVIRT_0.9.11 { virDomainPMWakeup; } LIBVIRT_0.9.10; +LIBVIRT_0.9.13 { + global: + virConnectListAllDomains; +} LIBVIRT_0.9.11; + # .... define new API here using predicted next version number .... -- 1.7.3.4

On 06/05/2012 07:19 AM, Peter Krempa wrote:
This patch adds a new public api that lists domains. The new approach is different from those used before. There are key points to this:
1) The list is acquired atomically and contains both active and inactive domains (guests). This eliminates the need to call two different list APIs, where the state might change in between of the calls.
s/between of/between/
2) The returned list consists of virDomainPtrs instead of names or ID's that have to be converted to virDomainPtrs anyways using separate calls for each one of them. This is more convenient and saves hypervisor calls.
3) The returned list is auto-allocated. This saves a lot hassle for the
s/lot/lot of/
users.
4) Built in support for filtering. The API call supports various filtering flags that modify the output list according to user needs.
Available filter groups: Domain status: VIR_CONNECT_LIST_DOMAINS_ACTIVE, VIR_CONNECT_LIST_DOMAINS_INACTIVE
Domain persistence: VIR_CONNECT_LIST_DOMAINS_PERSISTENT, VIR_CONNECT_LIST_DOMAINS_TRANSIENT
Domain state: VIR_CONNECT_LIST_DOMAINS_RUNNING, VIR_CONNECT_LIST_DOMAINS_PAUSED, VIR_CONNECT_LIST_DOMAINS_SHUTOFF, VIR_CONNECT_LIST_DOMAINS_OTHER
Presense of managed save image:
s/Presense/Presence/
VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE, VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE
Auto-start option: VIR_CONNECT_LIST_DOMAINS_AUTOSTART, VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART
Presense of snapshot:
s/Presense/Presence/
VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT, VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT
5) The python binding returns a list of domain objects that is very neat to work with.
The only problem with this approach is no support from code generators so both RPC code and python bindings had to be written manualy.
s/manualy/manually/
*include/libvirt/libvirt.h.in: - add API prototype - clean up whitespace mistakes nearby *python/generator.py: - inhibit generation of the bindings for the new api *src/driver.h: - add driver prototype - clean up some whitespace mistakes nearby *src/libvirt.c: - add public implementation *src/libvirt_public.syms: - export the new symbol --- Diff to v1: - added docs and filtering flags --- include/libvirt/libvirt.h.in | 36 ++++++++++++- python/generator.py | 1 + src/driver.h | 11 +++- src/libvirt.c | 122 +++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 5 ++ 5 files changed, 168 insertions(+), 7 deletions(-)
@@ -866,6 +870,7 @@ struct _virDriver { virDrvGetCapabilities getCapabilities; virDrvListDomains listDomains; virDrvNumOfDomains numOfDomains; + virDrvListAllDomains listAllDomains;
Spacing looks inconsistent here. (Hmm, changing things to use space instead of tab would be a separate commit.)
/** + * virConnectListAllDomains: + * @conn: Pointer to the hypervisor connection. + * @domains: Pointer to a variable to store the array containing domain objects + * or NULL if the list is not required (just returns number of guests). + * @flags: bitwise-OR of virConnectListAllDomainsFlags + * + * Collect a possibly-filtered list of all domains, and return an allocated + * array of information for each. This API solves the race inherent in + * virConnectListDomains() and virConnectListDefinedDomains(). + * + * Normally, all domains are returned; however, @flags can be used to + * filter the results for a smaller list of targetted domains. The valid
s/targetted/targeted/
+ * + * Returns the number of domains found or -1 in case of error. On success, + * the array stored into @doms is guaranteed to have an extra allocated + * element set to NULL, to make iteration easier.
Maybe: s/an extra allocated element set to NULL/& but not included in the return count/ to make it obvious that the trailing NULL does not impact the return value.
+ * + * Example of usage: + * virDomainPtr *domains; + * virDomainPtr dom; + * int i; + *
I'd drop this blank line.
+ * int ret; + * unsigned int flags = VIR_CONNECT_LIST_RUNNING | + * VIR_CONNECT_LIST_PERSISTENT; + * + * ret = virConnectListAllDomains(conn, &domains, flags); + * if (ret < 0) + * error(); + * + * for (i = 0; i < ret; i++) { + * do_someting_with_domain(domains[i]);
s/someting/something/
+{ + VIR_DEBUG("conn=%p, domains=%p, flags=%x", conn, domains, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + }
There's now a conflict between your commit and danpb's cleanups to error reporting in libvirt.c - whoever commits second should make sure to rebase properly. ACK with doc tweaks included. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch adds export of the new API function virConnectListAllDomains() to the libvirt-python bindings. The virConnect object now has method "listAllDomains" that takes only the flags parameter and returns a python list of virDomain object corresponding to virDomainPtrs returned by the underlying api. The implementation is done manually as the generator does not support wrapping list of virDomainPtrs into virDomain objects. --- No change to v1. --- python/libvirt-override-api.xml | 12 ++++++-- python/libvirt-override-virConnect.py | 12 ++++++++ python/libvirt-override.c | 47 ++++++++++++++++++++++++++++++++- 3 files changed, 67 insertions(+), 4 deletions(-) diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index 0bafd21..2fd6dec 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -19,17 +19,23 @@ <function name='virConnectListDefinedDomains' file='python'> <info>list the defined domains, stores the pointers to the names in @names</info> <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> - <return type='str *' info='the list of Names of None in case of error'/> + <return type='str *' info='the list of Names or None in case of error'/> + </function> + <function name='virConnectListAllDomains' file='python'> + <info>returns list of all defined domains</info> + <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> + <arg name='flags' type='unsigned int' info='optional flags'/> + <return type='domain *' info='the list of domains or None in case of error'/> </function> <function name='virConnectListNetworks' file='python'> <info>list the networks, stores the pointers to the names in @names</info> <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> - <return type='str *' info='the list of Names of None in case of error'/> + <return type='str *' info='the list of Names or None in case of error'/> </function> <function name='virConnectListDefinedNetworks' file='python'> <info>list the defined networks, stores the pointers to the names in @names</info> <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> - <return type='str *' info='the list of Names of None in case of error'/> + <return type='str *' info='the list of Names or None in case of error'/> </function> <function name='virDomainLookupByUUID' file='python'> <info>Try to lookup a domain on the given hypervisor based on its UUID.</info> diff --git a/python/libvirt-override-virConnect.py b/python/libvirt-override-virConnect.py index 811e16b..ecb5680 100644 --- a/python/libvirt-override-virConnect.py +++ b/python/libvirt-override-virConnect.py @@ -185,3 +185,15 @@ raise libvirtError ('virConnectDomainEventRegisterAny() failed', conn=self) self.domainEventCallbackID[ret] = opaque return ret + + def listAllDomains(self, flags): + """List all domains and returns a list of domain objects""" + ret = libvirtmod.virConnectListAllDomains(self._o, flags) + if ret is None: + raise libvirtError("virConnectListAllDomains() failed", conn=self) + + retlist = list() + for domptr in ret: + retlist.append(virDomain(self, _obj=domptr)) + + return retlist diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 130e702..6278578 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -1944,7 +1944,7 @@ libvirt_virConnectGetLibVersion (PyObject *self ATTRIBUTE_UNUSED, static PyObject * libvirt_virConnectListDomainsID(PyObject *self ATTRIBUTE_UNUSED, - PyObject *args) { + PyObject *args) { PyObject *py_retval; int *ids = NULL, c_retval, i; virConnectPtr conn; @@ -1986,6 +1986,50 @@ libvirt_virConnectListDomainsID(PyObject *self ATTRIBUTE_UNUSED, } static PyObject * +libvirt_virConnectListAllDomains(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + PyObject *pyobj_conn; + PyObject *py_retval = NULL; + virConnectPtr conn; + virDomainPtr *doms = NULL; + int c_retval = 0; + int i; + unsigned int flags; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virConnectListAllDomains", + &pyobj_conn, &flags)) + return NULL; + conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virConnectListAllDomains(conn, &doms, flags); + LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) + return VIR_PY_NONE; + + if (!(py_retval = PyList_New(c_retval))) + goto cleanup; + + for (i = 0; i < c_retval; i++) { + if (PyList_SetItem(py_retval, i, + libvirt_virDomainPtrWrap(doms[i])) < 0) { + Py_DECREF(py_retval); + py_retval = NULL; + goto cleanup; + } + doms[i] = NULL; + } + +cleanup: + for (i = 0; i < c_retval; i++) + if (doms[i]) + virDomainFree(doms[i]); + VIR_FREE(doms); + return py_retval; +} + +static PyObject * libvirt_virConnectListDefinedDomains(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { PyObject *py_retval; @@ -5618,6 +5662,7 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virConnectOpenAuth", libvirt_virConnectOpenAuth, METH_VARARGS, NULL}, {(char *) "virConnectListDomainsID", libvirt_virConnectListDomainsID, METH_VARARGS, NULL}, {(char *) "virConnectListDefinedDomains", libvirt_virConnectListDefinedDomains, METH_VARARGS, NULL}, + {(char *) "virConnectListAllDomains", libvirt_virConnectListAllDomains, METH_VARARGS, NULL}, {(char *) "virConnectDomainEventRegister", libvirt_virConnectDomainEventRegister, METH_VARARGS, NULL}, {(char *) "virConnectDomainEventDeregister", libvirt_virConnectDomainEventDeregister, METH_VARARGS, NULL}, {(char *) "virConnectDomainEventRegisterAny", libvirt_virConnectDomainEventRegisterAny, METH_VARARGS, NULL}, -- 1.7.3.4

On 06/05/2012 07:19 AM, Peter Krempa wrote:
This patch adds export of the new API function virConnectListAllDomains() to the libvirt-python bindings. The virConnect object now has method "listAllDomains" that takes only the flags parameter and returns a python list of virDomain object corresponding to virDomainPtrs returned by the underlying api.
The implementation is done manually as the generator does not support wrapping list of virDomainPtrs into virDomain objects. --- No change to v1. --- python/libvirt-override-api.xml | 12 ++++++-- python/libvirt-override-virConnect.py | 12 ++++++++ python/libvirt-override.c | 47 ++++++++++++++++++++++++++++++++- 3 files changed, 67 insertions(+), 4 deletions(-)
+ if (!(py_retval = PyList_New(c_retval))) + goto cleanup; + + for (i = 0; i < c_retval; i++) { + if (PyList_SetItem(py_retval, i, + libvirt_virDomainPtrWrap(doms[i])) < 0) {
libvirt_virDomainPtrWrap() can return NULL on OOM; but PyList_SetItem() cannot take a NULL argument. You need to separate this into two steps with an intermediate variable which you check for NULL. if ((tmp = libvirt_virDomainPtrWrap(doms[i])) == NULL || PyList_SetItem(py_retval, i, tmp) < 0) {
+ Py_DECREF(py_retval); + py_retval = NULL;
Likewise, to avoid a leak, you would need to add: Py_XDECREF(tmp); ACK with that fix. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch wires up the RPC protocol handlers for virConnectListAllDomains(). The RPC generator has no support for the way how virConnectListAllDomains() returns the results so the handler code had to be done manually. The new api is handled by REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS, with number 271 and marked with high priority. --- Diff to v1: Add the NULL element at the end of the list. --- daemon/remote.c | 52 ++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 63 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 14 ++++++++- src/remote_protocol-structs | 12 ++++++++ 4 files changed, 140 insertions(+), 1 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index a02c09b..2453e77 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -996,6 +996,58 @@ no_memory: } static int +remoteDispatchConnectListAllDomains(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_list_all_domains_args *args, + remote_connect_list_all_domains_ret *ret) +{ + virDomainPtr *doms = NULL; + int ndomains = 0; + int i; + int rv = -1; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if ((ndomains = virConnectListAllDomains(priv->conn, + args->need_results ? &doms : NULL, + args->flags)) < 0) + goto cleanup; + + if (doms) { + if (VIR_ALLOC_N(ret->domains.domains_val, ndomains) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret->domains.domains_len = ndomains; + + for (i = 0; i < ndomains; i++) + make_nonnull_domain(ret->domains.domains_val+i, doms[i]); + } else { + ret->domains.domains_len = 0; + ret->domains.domains_val = NULL; + } + + rv = ndomains; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (doms) { + for (i = 0; i < ndomains;i++) + virDomainFree(doms[i]); + VIR_FREE(doms); + } + return rv; +} + +static int remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, virNetMessagePtr msg ATTRIBUTE_UNUSED, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 299cd69..acf99d5 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1265,6 +1265,68 @@ done: return rv; } +static int +remoteConnectListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + int rv = -1; + int i; + virDomainPtr *doms = NULL; + remote_connect_list_all_domains_args args; + remote_connect_list_all_domains_ret ret; + + struct private_data *priv = conn->privateData; + + remoteDriverLock(priv); + + args.need_results = !!domains; + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + if (call (conn, + priv, + 0, + REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS, + (xdrproc_t) xdr_remote_connect_list_all_domains_args, + (char *) &args, + (xdrproc_t) xdr_remote_connect_list_all_domains_ret, + (char *) &ret) == -1) + goto done; + + if (domains) { + if (VIR_ALLOC_N(doms, ret.domains.domains_len + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + for (i = 0; i < ret.domains.domains_len; i++) { + doms[i] = get_nonnull_domain(conn, ret.domains.domains_val[i]); + if (!doms[i]) { + virReportOOMError(); + goto cleanup; + } + } + *domains = doms; + doms = NULL; + } + + rv = ret.domains.domains_len; + +cleanup: + if (doms) { + for (i = 0; i < ret.domains.domains_len; i++) + if (doms[i]) + virDomainFree(doms[i]); + VIR_FREE(doms); + } + + xdr_free((xdrproc_t) xdr_remote_connect_list_all_domains_ret, (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + /* Helper to free typed parameters. */ static void remoteFreeTypedParameters(remote_typed_param *args_params_val, @@ -5111,6 +5173,7 @@ static virDriver remote_driver = { .domainGetDiskErrors = remoteDomainGetDiskErrors, /* 0.9.10 */ .domainSetMetadata = remoteDomainSetMetadata, /* 0.9.10 */ .domainGetMetadata = remoteDomainGetMetadata, /* 0.9.10 */ + .listAllDomains = remoteConnectListAllDomains, /* 0.9.13 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 2d57247..ce7d053 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2463,6 +2463,16 @@ struct remote_domain_get_disk_errors_ret { int nerrors; }; +struct remote_connect_list_all_domains_args { + bool need_results; + unsigned int flags; +}; + +struct remote_connect_list_all_domains_ret { + remote_nonnull_domain domains<>; + unsigned int ret; +}; + /*----- Protocol. -----*/ @@ -2782,7 +2792,9 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_PM_WAKEUP = 267, /* autogen autogen */ REMOTE_PROC_DOMAIN_EVENT_TRAY_CHANGE = 268, /* autogen autogen */ REMOTE_PROC_DOMAIN_EVENT_PMWAKEUP = 269, /* autogen autogen */ - REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND = 270 /* autogen autogen */ + REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND = 270, /* autogen autogen */ + + REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS = 271 /* skipgen skipgen priority:high */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 9b2414f..839c348 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1921,6 +1921,17 @@ struct remote_domain_get_disk_errors_ret { } errors; int nerrors; }; +struct remote_connect_list_all_domains_args { + bool need_results; + u_int flags; +}; +struct remote_connect_list_all_domains_ret { + struct { + u_int domains_len; + remote_nonnull_domain * domains_val; + } domains; + u_int ret; +}; enum remote_procedure { REMOTE_PROC_OPEN = 1, REMOTE_PROC_CLOSE = 2, @@ -2192,4 +2203,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_EVENT_TRAY_CHANGE = 268, REMOTE_PROC_DOMAIN_EVENT_PMWAKEUP = 269, REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND = 270, + REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS = 271, }; -- 1.7.3.4

On 06/05/2012 07:19 AM, Peter Krempa wrote:
This patch wires up the RPC protocol handlers for virConnectListAllDomains(). The RPC generator has no support for the way how virConnectListAllDomains() returns the results so the handler code had to be done manually.
We might end up teaching the generator how to do this, as we add more ListAll* commands (for example, I'm in the middle of writing the series for virDomainListAllSnapshots, using this series as my template), but updating the generator is a story for another day.
The new api is handled by REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS, with number 271 and marked with high priority. --- Diff to v1: Add the NULL element at the end of the list. --- daemon/remote.c | 52 ++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 63 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 14 ++++++++- src/remote_protocol-structs | 12 ++++++++ 4 files changed, 140 insertions(+), 1 deletions(-)
+++ b/daemon/remote.c @@ -996,6 +996,58 @@ no_memory: }
static int +remoteDispatchConnectListAllDomains(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_list_all_domains_args *args, + remote_connect_list_all_domains_ret *ret) +{
+ if ((ndomains = virConnectListAllDomains(priv->conn, + args->need_results ? &doms : NULL, + args->flags)) < 0) + goto cleanup; + + if (doms) {
Use 'if (doms && ndomains)' to avoid a pointless 0-length array allocation.
+ if (VIR_ALLOC_N(ret->domains.domains_val, ndomains) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret->domains.domains_len = ndomains; + + for (i = 0; i < ndomains; i++) + make_nonnull_domain(ret->domains.domains_val+i, doms[i]);
Style nit - spaces around '+'. Ouch - we have a latent bug. make_nonnull_domain and friends do strdup() without checking for NULL. Not your fault, but we should really clean that up to trigger the OOM handler instead.
+ } else { + ret->domains.domains_len = 0; + ret->domains.domains_val = NULL; + } + + rv = ndomains; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (doms) { + for (i = 0; i < ndomains;i++)
Style nit - space after second ';'.
+++ b/src/remote/remote_protocol.x @@ -2463,6 +2463,16 @@ struct remote_domain_get_disk_errors_ret { int nerrors; };
+struct remote_connect_list_all_domains_args { + bool need_results;
s/bool/int/ - I'm not sure that rpcgen will gracefully handle 'bool' on all platforms (just because glibc supports it doesn't make it valid XDR notation everywhere else).
+++ b/src/remote_protocol-structs @@ -1921,6 +1921,17 @@ struct remote_domain_get_disk_errors_ret { } errors; int nerrors; }; +struct remote_connect_list_all_domains_args { + bool need_results;
If you have dwarves-1.9 installed, it's broken, hence you were not actually testing this file. Upgrade to dwarves-1.10. If you have dwarves-1.10 installed, you will notice that this shows up as 'bool_t' and not bool (don't know why the debug information encodes things that way, but that's what we have to live with). All the more reason to use int, not bool, in the .x file. I'll go ahead and prepare a patch for the OOM bug in daemon/remote.c, but meanwhile, I think I can ACK this patch with nits fixed, without having to see a v3, since the changes are pretty small. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 06/05/2012 07:19 AM, Peter Krempa wrote:
This patch wires up the RPC protocol handlers for virConnectListAllDomains(). The RPC generator has no support for the way how virConnectListAllDomains() returns the results so the handler code had to be done manually.
The new api is handled by REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS, with number 271 and marked with high priority. --- Diff to v1: Add the NULL element at the end of the list.
I missed this in my earlier review:
+++ b/daemon/remote.c @@ -996,6 +996,58 @@ no_memory: }
static int +remoteDispatchConnectListAllDomains(virNetServerPtr server ATTRIBUTE_UNUSED,
+ } else { + ret->domains.domains_len = 0; + ret->domains.domains_val = NULL; + } + + rv = ndomains;
Here, you want: ret->ret = ndomains; rv = 0;
+++ b/src/remote/remote_driver.c @@ -1265,6 +1265,68 @@ done: return rv; }
+static int +remoteConnectListAllDomains(virConnectPtr conn,
+ if (call (conn, + priv, + 0, + REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS, + (xdrproc_t) xdr_remote_connect_list_all_domains_args, + (char *) &args, + (xdrproc_t) xdr_remote_connect_list_all_domains_ret, + (char *) &ret) == -1) + goto done; + + if (domains) { + if (VIR_ALLOC_N(doms, ret.domains.domains_len + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + for (i = 0; i < ret.domains.domains_len; i++) { + doms[i] = get_nonnull_domain(conn, ret.domains.domains_val[i]); + if (!doms[i]) { + virReportOOMError(); + goto cleanup; + } + } + *domains = doms; + doms = NULL; + } + + rv = ret.domains.domains_len;
and here, you want: rv = ret.ret;
+++ b/src/remote/remote_protocol.x @@ -2463,6 +2463,16 @@ struct remote_domain_get_disk_errors_ret { int nerrors; };
+struct remote_connect_list_all_domains_args { + bool need_results; + unsigned int flags; +}; + +struct remote_connect_list_all_domains_ret { + remote_nonnull_domain domains<>; + unsigned int ret;
That's because the daemon/remote.c doesn't do any special treatment for positive return values; to specifically pass a positive count, you have to pass it through the remote_*_ret.ret member. If need_results was 0 in _args, then ret.domains.domains_len will be 0 in reply, even though ret should be non-zero for the count of domains. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch adds common code to list domains in fashion used by virListAllDomains and helpers to filter this list. This code supports filters based on data stored in the virDomainObj. Supported filter flags: VIR_CONNECT_LIST_DOMAINS_ACTIVE VIR_CONNECT_LIST_DOMAINS_INACTIVE VIR_CONNECT_LIST_DOMAINS_PERSISTENT VIR_CONNECT_LIST_DOMAINS_TRANSIENT VIR_CONNECT_LIST_DOMAINS_RUNNING VIR_CONNECT_LIST_DOMAINS_PAUSED VIR_CONNECT_LIST_DOMAINS_SHUTOFF VIR_CONNECT_LIST_DOMAINS_OTHER VIR_CONNECT_LIST_DOMAINS_AUTOSTART VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT Hypervisor specific flags have to be filtered after using this function. This patch adds virDomainListFilter() helper function to remove elements from the array based on a predicate. --- New in this series. --- src/Makefile.am | 6 +- src/conf/virdomainlist.c | 214 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/virdomainlist.h | 36 ++++++++ src/libvirt_private.syms | 5 + 4 files changed, 260 insertions(+), 1 deletions(-) create mode 100644 src/conf/virdomainlist.c create mode 100644 src/conf/virdomainlist.h diff --git a/src/Makefile.am b/src/Makefile.am index 5693fb4..fd9d892 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -194,6 +194,9 @@ CPU_CONF_SOURCES = \ CONSOLE_CONF_SOURCES = \ conf/virconsole.c conf/virconsole.h +# Domain listing helpers +DOMAIN_LIST_SOURCES = \ + conf/virdomainlist.c conf/virdomainlist.h CONF_SOURCES = \ $(NETDEV_CONF_SOURCES) \ $(DOMAIN_CONF_SOURCES) \ @@ -206,7 +209,8 @@ CONF_SOURCES = \ $(INTERFACE_CONF_SOURCES) \ $(SECRET_CONF_SOURCES) \ $(CPU_CONF_SOURCES) \ - $(CONSOLE_CONF_SOURCES) + $(CONSOLE_CONF_SOURCES) \ + $(DOMAIN_LIST_SOURCES) # The remote RPC driver, covering domains, storage, networks, etc REMOTE_DRIVER_GENERATED = \ diff --git a/src/conf/virdomainlist.c b/src/conf/virdomainlist.c new file mode 100644 index 0000000..180b37d --- /dev/null +++ b/src/conf/virdomainlist.c @@ -0,0 +1,214 @@ +/** + * virdomainlist.c: Helpers for listing and filtering domains. + * + * Copyright (C) 2011-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Peter Krempa <pkrempa@redhat.com> + */ + +#include <config.h> + +#include "virdomainlist.h" + +#include "internal.h" +#include "virhash.h" +#include "domain_conf.h" +#include "memory.h" +#include "datatypes.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +struct virDomainListData { + virConnectPtr conn; + virDomainPtr *domains; + unsigned int flags; + int ndomains; + size_t size; + bool error; +}; + +#define MATCH(FLAG) (data->flags & (FLAG)) +static void +virDomainListPopulate(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virDomainListData *data = opaque; + virDomainObjPtr vm = payload; + virDomainPtr dom; + + if (data->error) + return; + + virDomainObjLock(vm); + /* check if the domain matches the filter */ + + /* filter by active state */ + if (MATCH(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE) && + !((MATCH(VIR_CONNECT_LIST_DOMAINS_ACTIVE) && + virDomainObjIsActive(vm)) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_INACTIVE) && + !virDomainObjIsActive(vm)))) + goto cleanup; + + + /* filter by persistence */ + if (MATCH(VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT) && + !((MATCH(VIR_CONNECT_LIST_DOMAINS_PERSISTENT) && + vm->persistent) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_TRANSIENT) && + !vm->persistent))) + goto cleanup; + + /* filter by domain state */ + if (MATCH(VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_PAUSED | + VIR_CONNECT_LIST_DOMAINS_SHUTOFF | + VIR_CONNECT_LIST_DOMAINS_OTHER)) { + int st = virDomainObjGetState(vm, NULL); + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_RUNNING) && + st == VIR_DOMAIN_RUNNING) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_PAUSED) && + st == VIR_DOMAIN_PAUSED) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_SHUTOFF) && + st == VIR_DOMAIN_SHUTOFF) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_OTHER) && + (st == VIR_DOMAIN_NOSTATE || st == VIR_DOMAIN_BLOCKED || + st == VIR_DOMAIN_SHUTDOWN || st == VIR_DOMAIN_CRASHED || + st == VIR_DOMAIN_PMSUSPENDED)))) + goto cleanup; + } + + /* managed save filter has to be driver specific */ + + /* filter by autostart option */ + if (MATCH(VIR_CONNECT_LIST_DOMAINS_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART) && + !((MATCH(VIR_CONNECT_LIST_DOMAINS_AUTOSTART) && vm->autostart) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART) && !vm->autostart))) + goto cleanup; + + /* filter by snapshot existence */ + if (MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT)) { + int nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0); + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT) && + nsnapshots > 0) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT) && + nsnapshots <= 0))) + goto cleanup; + } + + /* just count the machines */ + if (!data->domains) { + data->ndomains++; + return; + } + + /* add the domain to the result list */ + if (VIR_EXPAND_N(data->domains, data->size, 1) < 0) + goto no_memory; + + if (!(dom = virGetDomain(data->conn, vm->def->name, vm->def->uuid))) + goto no_memory; + + dom->id = vm->def->id; + + data->domains[data->ndomains++] = dom; + +cleanup: + virDomainObjUnlock(vm); + return; + +no_memory: + virReportOOMError(); + data->error = true; + goto cleanup; +} +#undef MATCH + +int +virDomainList(virConnectPtr conn, virHashTablePtr domobjs, + virDomainPtr **domains, unsigned int flags) +{ + int ret = -1; + int i; + + struct virDomainListData data = { conn, NULL, flags, 0, 0, false }; + + if (domains) { + if (VIR_ALLOC_N(data.domains, 1) < 0) { + virReportOOMError(); + goto cleanup; + } + data.size = 1; + } + + virHashForEach(domobjs, virDomainListPopulate, &data); + + if (data.error) + goto cleanup; + + if (data.domains) { + *domains = data.domains; + data.domains = NULL; + } + + ret = data.ndomains; + +cleanup: + if (data.domains) { + for (i = 0; i < data.size; i++) + if (data.domains[i]) + virDomainFree(data.domains[i]); + VIR_FREE(data.domains); + } + + return ret; +} + +int +virDomainListFilter(virDomainPtr **doms, + int *ndoms, + virDomainListFilterFunc filter, + int result) +{ + int i = 0; + int ret; + + while (i < *ndoms) { + ret = filter((*doms)[i]); + if (ret < 0) + return -1; + + if (ret == result) { + virDomainFree((*doms)[i]); + if (i != --*ndoms) + memmove(&(*doms)[i], &(*doms)[i+1], sizeof(*doms) * (*ndoms - i)); + + if (VIR_REALLOC_N(*doms, *ndoms) < 0) { + ; /* not fatal */ + } + continue; + } + i++; + } + return 0; +} diff --git a/src/conf/virdomainlist.h b/src/conf/virdomainlist.h new file mode 100644 index 0000000..80fe4f8 --- /dev/null +++ b/src/conf/virdomainlist.h @@ -0,0 +1,36 @@ +/** + * virdomainlist.h: Helpers for listing and filtering domains. + * + * Copyright (C) 2011-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Peter Krempa <pkrempa@redhat.com> + */ +#ifndef __VIR_DOMAIN_LIST_H__ +# define __VIR_DOMAIN_LIST_H__ + +# include "internal.h" +# include "virhash.h" + +typedef int (*virDomainListFilterFunc)(virDomainPtr dom); + +int virDomainListFilter(virDomainPtr **doms, int *ndoms, + virDomainListFilterFunc filter, int result); + +int virDomainList(virConnectPtr conn, virHashTablePtr domobjs, + virDomainPtr **domains, unsigned int flags); + +#endif /* __VIR_DOMAIN_LIST_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fdf2186..eacdf3a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1237,6 +1237,11 @@ virConsoleOpen; virDBusGetSystemBus; +# virdomainlist.h +virDomainList; +virDomainListFilter; + + # virfile.h virFileClose; virFileDirectFdFlag; -- 1.7.3.4

On 06/05/2012 07:19 AM, Peter Krempa wrote:
This patch adds common code to list domains in fashion used by virListAllDomains and helpers to filter this list.
This code supports filters based on data stored in the virDomainObj. Supported filter flags: VIR_CONNECT_LIST_DOMAINS_ACTIVE VIR_CONNECT_LIST_DOMAINS_INACTIVE VIR_CONNECT_LIST_DOMAINS_PERSISTENT VIR_CONNECT_LIST_DOMAINS_TRANSIENT VIR_CONNECT_LIST_DOMAINS_RUNNING VIR_CONNECT_LIST_DOMAINS_PAUSED VIR_CONNECT_LIST_DOMAINS_SHUTOFF VIR_CONNECT_LIST_DOMAINS_OTHER VIR_CONNECT_LIST_DOMAINS_AUTOSTART VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT
Hypervisor specific flags have to be filtered after using this function. This patch adds virDomainListFilter() helper function to remove elements from the array based on a predicate.
You know, the only flag group you don't have here is DOMAINS_[NO_]MANAGEDSAVE, but it wouldn't be that hard to update virDomainObjPtr to have a new bool field that tracks this information (update the information when a managedsave image is created, deleted, and when reloading domain state at libvirtd startup). Besides, we already have precedence for that - it is how we manage domain snapshots (that is, qemu_driver.c updates vm->snapshots at key points in snapshot life cycle). I'd rather see a pre-requisite patch that adds a new 'has_managedsave' bool flag to the struct, at which point [0]...
--- New in this series. --- src/Makefile.am | 6 +- src/conf/virdomainlist.c | 214 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/virdomainlist.h | 36 ++++++++ src/libvirt_private.syms | 5 + 4 files changed, 260 insertions(+), 1 deletions(-) create mode 100644 src/conf/virdomainlist.c create mode 100644 src/conf/virdomainlist.h
diff --git a/src/Makefile.am b/src/Makefile.am index 5693fb4..fd9d892 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -194,6 +194,9 @@ CPU_CONF_SOURCES = \ CONSOLE_CONF_SOURCES = \ conf/virconsole.c conf/virconsole.h
+# Domain listing helpers +DOMAIN_LIST_SOURCES = \ + conf/virdomainlist.c conf/virdomainlist.h CONF_SOURCES = \
While what you have works, I like to add a blank line between any macro definition that uses \-newline to take up more than one source line, so that it is a bit more obvious that the last line should not have \ (or if someone does accidentally put \ on the last line, at least the next macro name is still an independent macro name rather than an unintended continuation of the first macro). That said...
$(NETDEV_CONF_SOURCES) \ $(DOMAIN_CONF_SOURCES) \ @@ -206,7 +209,8 @@ CONF_SOURCES = \ $(INTERFACE_CONF_SOURCES) \ $(SECRET_CONF_SOURCES) \ $(CPU_CONF_SOURCES) \ - $(CONSOLE_CONF_SOURCES) + $(CONSOLE_CONF_SOURCES) \ + $(DOMAIN_LIST_SOURCES)
...I would just inline the listing of the two new files to DOMAIN_CONF_SOURCES rather than creating a new category DOMAIN_LIST_SOURCES.
# The remote RPC driver, covering domains, storage, networks, etc REMOTE_DRIVER_GENERATED = \ diff --git a/src/conf/virdomainlist.c b/src/conf/virdomainlist.c new file mode 100644 index 0000000..180b37d --- /dev/null +++ b/src/conf/virdomainlist.c @@ -0,0 +1,214 @@ +/** + * virdomainlist.c: Helpers for listing and filtering domains. + * + * Copyright (C) 2011-2012 Red Hat, Inc.
Are we really borrowing any contents written in 2011, or can this be shortened to just 2012?
+#define VIR_FROM_THIS VIR_FROM_NONE
s/VIR_FROM_NONE/VIR_FROM_DOMAIN/
+ +struct virDomainListData { + virConnectPtr conn; + virDomainPtr *domains; + unsigned int flags; + int ndomains; + size_t size;
I would consider adding a second size_t variable, so that you can track usage independently from allocations. In other words, the number of domains can be large enough that reallocating the array for every member can be noticeably slower than reallocating the array geometrically [1]. Or, don't even bother with reallocations [2].
+ /* filter by domain state */ + if (MATCH(VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_PAUSED | + VIR_CONNECT_LIST_DOMAINS_SHUTOFF | + VIR_CONNECT_LIST_DOMAINS_OTHER)) { + int st = virDomainObjGetState(vm, NULL); + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_RUNNING) && + st == VIR_DOMAIN_RUNNING) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_PAUSED) && + st == VIR_DOMAIN_PAUSED) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_SHUTOFF) && + st == VIR_DOMAIN_SHUTOFF) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_OTHER) && + (st == VIR_DOMAIN_NOSTATE || st == VIR_DOMAIN_BLOCKED || + st == VIR_DOMAIN_SHUTDOWN || st == VIR_DOMAIN_CRASHED || + st == VIR_DOMAIN_PMSUSPENDED))))
Rather than listing all the other states here (and missing something when a new state is added), I would write this clause as: (MATCH(VIR_CONNECT_LIST_DOMAINS_OTHER) && st != VIR_DOMAIN_RUNNING && st != VIR_DOMAIN_PAUSED && st != VIR_DOMAIN_SHUTOFF)
+ + /* filter by snapshot existence */ + if (MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT)) { + int nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0); + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT) && + nsnapshots > 0) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT) && + nsnapshots <= 0))) + goto cleanup; + }
Careful. ESX and VBox support snapshots, but do not track them in &vm->snapshots. We can keep this common filtering only if those two hypervisors mask out both HAS_SNAPSHOT and NO_SNAPSHOT bits before calling into this function. Then again, those two hypervisors don't use virDomainObj in the first place, so maybe I'm getting ahead of myself.
+ + /* add the domain to the result list */ + if (VIR_EXPAND_N(data->domains, data->size, 1) < 0)
[1] Here, I'd use the four-argument VIR_RESIZE_N, for geometric growth of the array, if we even keep the realloc as part of this callback.
+int +virDomainList(virConnectPtr conn, virHashTablePtr domobjs, + virDomainPtr **domains, unsigned int flags) +{ + int ret = -1; + int i; + + struct virDomainListData data = { conn, NULL, flags, 0, 0, false };
Why the double space before NULL?
+ + if (domains) { + if (VIR_ALLOC_N(data.domains, 1) < 0) {
[2] Why not just allocate the end result according to the size of the hash table? We know that the result cannot exceed virHashSize(domobjs) (it might be smaller, but if we overallocate now, then we don't have to mess with realloc in the middle of each callback).
+ +int +virDomainListFilter(virDomainPtr **doms, + int *ndoms, + virDomainListFilterFunc filter, + int result) +{
[0]... we might not even need this function, if you can get the common virDomainObjPtr code to track managedsave images in place (at least, that was the only reason that 5/9 used it; I didn't check if 6/9 or 7/9 needed to use it for other reasons).
+ int i = 0; + int ret; + + while (i < *ndoms) { + ret = filter((*doms)[i]); + if (ret < 0) + return -1; + + if (ret == result) { + virDomainFree((*doms)[i]); + if (i != --*ndoms) + memmove(&(*doms)[i], &(*doms)[i+1], sizeof(*doms) * (*ndoms - i)); + + if (VIR_REALLOC_N(*doms, *ndoms) < 0) {
VIR_SHRINK_N might be nicer here.
diff --git a/src/conf/virdomainlist.h b/src/conf/virdomainlist.h new file mode 100644 index 0000000..80fe4f8 --- /dev/null +++ b/src/conf/virdomainlist.h @@ -0,0 +1,36 @@ +/** + * virdomainlist.h: Helpers for listing and filtering domains. + * + * Copyright (C) 2011-2012 Red Hat, Inc.
Same question on copyright year. Worth another round of review on this patch. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 06/06/12 00:29, Eric Blake wrote:
diff --git a/src/Makefile.am b/src/Makefile.am index 5693fb4..fd9d892 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -194,6 +194,9 @@ CPU_CONF_SOURCES = \ CONSOLE_CONF_SOURCES = \ conf/virconsole.c conf/virconsole.h
+# Domain listing helpers +DOMAIN_LIST_SOURCES = \ + conf/virdomainlist.c conf/virdomainlist.h CONF_SOURCES = \
While what you have works, I like to add a blank line between any macro definition that uses \-newline to take up more than one source line, so that it is a bit more obvious that the last line should not have \ (or if someone does accidentally put \ on the last line, at least the next macro name is still an independent macro name rather than an unintended continuation of the first macro). That said...
$(NETDEV_CONF_SOURCES) \ $(DOMAIN_CONF_SOURCES) \ @@ -206,7 +209,8 @@ CONF_SOURCES = \ $(INTERFACE_CONF_SOURCES) \ $(SECRET_CONF_SOURCES) \ $(CPU_CONF_SOURCES) \ - $(CONSOLE_CONF_SOURCES) + $(CONSOLE_CONF_SOURCES) \ + $(DOMAIN_LIST_SOURCES)
...I would just inline the listing of the two new files to DOMAIN_CONF_SOURCES rather than creating a new category DOMAIN_LIST_SOURCES.
The problem with DOMAIN_CONF_SOURCES is that files specified there get built into the libvirt_lxc binary that doesn't include/link libvirt.[ch] which results into a compile failure as virGetDomain is undefined in that case. That's also the reason I used CONSOLE_CONF_SOURCES.
# The remote RPC driver, covering domains, storage, networks, etc REMOTE_DRIVER_GENERATED = \ diff --git a/src/conf/virdomainlist.c b/src/conf/virdomainlist.c new file mode 100644 index 0000000..180b37d --- /dev/null +++ b/src/conf/virdomainlist.c @@ -0,0 +1,214 @@ +/** + * virdomainlist.c: Helpers for listing and filtering domains. + * + * Copyright (C) 2011-2012 Red Hat, Inc.
Are we really borrowing any contents written in 2011, or can this be shortened to just 2012?
I just borrowed the license statement and forgot to fix the year :) Peter

This patch adds support for listing all domains into drivers that use the common virDomainObj implementation: libxl, lxc, openvz, qemu, test, uml, vmware. The qemu driver has extra support for filtering by managed save image existence. --- New in series. I couldn't test libxl, openvz, uml and vmware, but they share the implementation, so they should work. --- src/libxl/libxl_driver.c | 33 +++++++++++++++++ src/lxc/lxc_driver.c | 32 ++++++++++++++++ src/openvz/openvz_driver.c | 30 +++++++++++++++ src/qemu/qemu_driver.c | 87 ++++++++++++++++++++++++++++++++++++++++---- src/test/test_driver.c | 31 ++++++++++++++++ src/uml/uml_driver.c | 31 ++++++++++++++++ src/vmware/vmware_driver.c | 31 ++++++++++++++++ 7 files changed, 268 insertions(+), 7 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 500d51b..00410f9 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -45,6 +45,7 @@ #include "xen_xm.h" #include "virtypedparam.h" #include "viruri.h" +#include "virdomainlist.h" #define VIR_FROM_THIS VIR_FROM_LIBXL @@ -3831,6 +3832,37 @@ libxlIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) return 1; } +static int +libxlListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + libxlDriverPrivatePtr driver = conn->privateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT | + VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_PAUSED | + VIR_CONNECT_LIST_DOMAINS_SHUTOFF | + VIR_CONNECT_LIST_DOMAINS_OTHER | + VIR_CONNECT_LIST_DOMAINS_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, -1); + + libxlDriverLock(driver); + + ret = virDomainList(conn, driver->domains.objs, domains, flags); + + libxlDriverUnlock(driver); + + return ret; +} + + static virDriver libxlDriver = { .no = VIR_DRV_LIBXL, @@ -3907,6 +3939,7 @@ static virDriver libxlDriver = { .domainEventRegisterAny = libxlDomainEventRegisterAny, /* 0.9.0 */ .domainEventDeregisterAny = libxlDomainEventDeregisterAny, /* 0.9.0 */ .isAlive = libxlIsAlive, /* 0.9.8 */ + .listAllDomains = libxlListAllDomains, /* 0.9.13 */ }; static virStateDriver libxlStateDriver = { diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 919f4ab..099e9bc 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -61,6 +61,7 @@ #include "virtime.h" #include "virtypedparam.h" #include "viruri.h" +#include "virdomainlist.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -3821,6 +3822,36 @@ cleanup: } static int +lxcListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + lxc_driver_t *driver = conn->privateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT | + VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_PAUSED | + VIR_CONNECT_LIST_DOMAINS_SHUTOFF | + VIR_CONNECT_LIST_DOMAINS_OTHER | + VIR_CONNECT_LIST_DOMAINS_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, -1); + + lxcDriverLock(driver); + + ret = virDomainList(conn, driver->domains.objs, domains, flags); + + lxcDriverUnlock(driver); + + return ret; +} + +static int lxcVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED, virHashIterator iter, void *data) { @@ -3912,6 +3943,7 @@ static virDriver lxcDriver = { .domainOpenConsole = lxcDomainOpenConsole, /* 0.8.6 */ .isAlive = lxcIsAlive, /* 0.9.8 */ .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ + .listAllDomains = lxcListAllDomains, /* 0.9.13 */ }; static virStateDriver lxcStateDriver = { diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index fb72cde..12baf90 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -60,6 +60,7 @@ #include "command.h" #include "viruri.h" #include "stats_linux.h" +#include "virdomainlist.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ @@ -1945,6 +1946,34 @@ cleanup: return ret; } +static int +openvzListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + struct openvz_driver *driver = conn->privateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT | + VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_PAUSED | + VIR_CONNECT_LIST_DOMAINS_SHUTOFF | + VIR_CONNECT_LIST_DOMAINS_OTHER | + VIR_CONNECT_LIST_DOMAINS_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, -1); + + openvzDriverLock(driver); + ret = virDomainList(conn, driver->domains.objs, domains, flags); + openvzDriverUnlock(driver); + + return ret; +} + static virDriver openvzDriver = { .no = VIR_DRV_OPENVZ, @@ -2000,6 +2029,7 @@ static virDriver openvzDriver = { .domainIsPersistent = openvzDomainIsPersistent, /* 0.7.3 */ .domainIsUpdated = openvzDomainIsUpdated, /* 0.8.6 */ .isAlive = openvzIsAlive, /* 0.9.8 */ + .listAllDomains = openvzListAllDomains, /* 0.9.13 */ }; int openvzRegister(void) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3f74d2..dae76ee 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -92,6 +92,7 @@ #include "virnodesuspend.h" #include "virtime.h" #include "virtypedparam.h" +#include "virdomainlist.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -2919,12 +2920,29 @@ cleanup: } static int +qemuDomainObjHasManagedSaveImage(struct qemud_driver *driver, + virDomainObjPtr vm) +{ + char *name; + int ret; + + name = qemuDomainManagedSavePath(driver, vm); + if (name == NULL) + return -1; + + ret = virFileExists(name); + + VIR_FREE(name); + + return ret; +} + +static int qemuDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm = NULL; int ret = -1; - char *name = NULL; virCheckFlags(0, -1); @@ -2938,14 +2956,9 @@ qemuDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) goto cleanup; } - name = qemuDomainManagedSavePath(driver, vm); - if (name == NULL) - goto cleanup; - - ret = virFileExists(name); + ret = qemuDomainObjHasManagedSaveImage(driver, vm); cleanup: - VIR_FREE(name); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); @@ -12946,6 +12959,65 @@ cleanup: return ret; } +static int +qemuListAllDomainsManagedSaveFilter(virDomainPtr domain) +{ + struct qemud_driver *driver = domain->conn->privateData; + int ret; + + /* driver is already locked */ + virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, domain->uuid); + + ret = qemuDomainObjHasManagedSaveImage(driver, vm); + virDomainObjUnlock(vm); + return ret; +} + +static int +qemuListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + struct qemud_driver *driver = conn->privateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT | + VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_PAUSED | + VIR_CONNECT_LIST_DOMAINS_SHUTOFF | + VIR_CONNECT_LIST_DOMAINS_OTHER | + VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE | + VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE | + VIR_CONNECT_LIST_DOMAINS_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, -1); + + qemuDriverLock(driver); + + if ((ret = virDomainList(conn, driver->domains.objs, domains, flags)) < 0) + goto cleanup; + + /* driver specific filters */ + /* filter by managed save image */ + if (!(flags & VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE && + flags & VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE)) { + if (flags & VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE) + virDomainListFilter(domains, &ret, + qemuListAllDomainsManagedSaveFilter, 0); + if (flags & VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE) + virDomainListFilter(domains, &ret, + qemuListAllDomainsManagedSaveFilter, 1); + } + +cleanup: + qemuDriverUnlock(driver); + return ret; +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -13106,6 +13178,7 @@ static virDriver qemuDriver = { .domainPMSuspendForDuration = qemuDomainPMSuspendForDuration, /* 0.9.11 */ .domainPMWakeup = qemuDomainPMWakeup, /* 0.9.11 */ .domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.11 */ + .listAllDomains = qemuListAllDomains, /* 0.9.13 */ }; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 7038741..6ab1067 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -52,6 +52,7 @@ #include "virfile.h" #include "virtypedparam.h" #include "virrandom.h" +#include "virdomainlist.h" #define VIR_FROM_THIS VIR_FROM_TEST @@ -5517,6 +5518,35 @@ static int testNWFilterClose(virConnectPtr conn) { return 0; } +static int testListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + testConnPtr privconn = conn->privateData; + int ret; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT | + VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_PAUSED | + VIR_CONNECT_LIST_DOMAINS_SHUTOFF | + VIR_CONNECT_LIST_DOMAINS_OTHER | + VIR_CONNECT_LIST_DOMAINS_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, -1); + + + testDriverLock(privconn); + ret = virDomainList(conn, privconn->domains.objs, domains, flags); + testDriverUnlock(privconn); + + return ret; +} + + static virDriver testDriver = { .no = VIR_DRV_TEST, .name = "Test", @@ -5584,6 +5614,7 @@ static virDriver testDriver = { .domainEventRegisterAny = testDomainEventRegisterAny, /* 0.8.0 */ .domainEventDeregisterAny = testDomainEventDeregisterAny, /* 0.8.0 */ .isAlive = testIsAlive, /* 0.9.8 */ + .listAllDomains = testListAllDomains, /* 0.9.13 */ }; static virNetworkDriver testNetworkDriver = { diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 219246d..79306f3 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -64,6 +64,7 @@ #include "virnetdevtap.h" #include "virnodesuspend.h" #include "viruri.h" +#include "virdomainlist.h" #define VIR_FROM_THIS VIR_FROM_UML @@ -2519,6 +2520,35 @@ static void umlDomainEventQueue(struct uml_driver *driver, virDomainEventStateQueue(driver->domainEventState, event); } +static int umlListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + struct uml_driver *driver = conn->privateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT | + VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_PAUSED | + VIR_CONNECT_LIST_DOMAINS_SHUTOFF | + VIR_CONNECT_LIST_DOMAINS_OTHER | + VIR_CONNECT_LIST_DOMAINS_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, -1); + + umlDriverLock(driver); + + ret = virDomainList(conn, driver->domains.objs, domains, flags); + + umlDriverUnlock(driver); + + return ret; +} + static virDriver umlDriver = { @@ -2578,6 +2608,7 @@ static virDriver umlDriver = { .domainOpenConsole = umlDomainOpenConsole, /* 0.8.6 */ .isAlive = umlIsAlive, /* 0.9.8 */ .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ + .listAllDomains = umlListAllDomains, /* 0.9.13 */ }; static int diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 8f9d922..acf45b4 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -33,6 +33,7 @@ #include "vmx.h" #include "vmware_conf.h" #include "vmware_driver.h" +#include "virdomainlist.h" static const char *vmw_types[] = { "player", "ws" }; @@ -994,6 +995,35 @@ vmwareIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) return 1; } +static int +vmwareListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + struct vmware_driver *driver = conn->privateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT | + VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_PAUSED | + VIR_CONNECT_LIST_DOMAINS_SHUTOFF | + VIR_CONNECT_LIST_DOMAINS_OTHER | + VIR_CONNECT_LIST_DOMAINS_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, -1); + + vmwareDriverLock(driver); + ret = virDomainList(conn, driver->domains.objs, domains, flags); + vmwareDriverUnlock(driver); + return ret; +} + + + static virDriver vmwareDriver = { .no = VIR_DRV_VMWARE, .name = "VMWARE", @@ -1029,6 +1059,7 @@ static virDriver vmwareDriver = { .domainIsActive = vmwareDomainIsActive, /* 0.8.7 */ .domainIsPersistent = vmwareDomainIsPersistent, /* 0.8.7 */ .isAlive = vmwareIsAlive, /* 0.9.8 */ + .listAllDomains = vmwareListAllDomains, /* 0.9.13 */ }; int -- 1.7.3.4

On 06/05/2012 07:19 AM, Peter Krempa wrote:
This patch adds support for listing all domains into drivers that use the common virDomainObj implementation: libxl, lxc, openvz, qemu, test, uml, vmware.
The qemu driver has extra support for filtering by managed save image existence.
See my comments on 4/9 about adding a prereq patch that factors the existence of managed save into virDomainObjPtr, so that we can treat that flag by default for all of these drivers.
--- New in series. I couldn't test libxl, openvz, uml and vmware, but they share the implementation, so they should work.
I am likewise not set up to test those, but agree with the assessment that there's enough copy-and-paste here to be safe.
--- src/libxl/libxl_driver.c | 33 +++++++++++++++++ src/lxc/lxc_driver.c | 32 ++++++++++++++++ src/openvz/openvz_driver.c | 30 +++++++++++++++ src/qemu/qemu_driver.c | 87 ++++++++++++++++++++++++++++++++++++++++---- src/test/test_driver.c | 31 ++++++++++++++++ src/uml/uml_driver.c | 31 ++++++++++++++++ src/vmware/vmware_driver.c | 31 ++++++++++++++++ 7 files changed, 268 insertions(+), 7 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 500d51b..00410f9 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -45,6 +45,7 @@ #include "xen_xm.h" #include "virtypedparam.h" #include "viruri.h" +#include "virdomainlist.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -3831,6 +3832,37 @@ libxlIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) return 1; }
+static int +libxlListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + libxlDriverPrivatePtr driver = conn->privateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT | + VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_PAUSED | + VIR_CONNECT_LIST_DOMAINS_SHUTOFF | + VIR_CONNECT_LIST_DOMAINS_OTHER | + VIR_CONNECT_LIST_DOMAINS_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART | + VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, -1);
libxl has managed save support, so we should support the managedsave flags by default (again, an argument that the key points in the domain lifecycle for creating or deleting the managedsave image should update the virDomainObjPtr, as well as libvirtd reloads). This is a pretty long list of flags, repeated among several drivers. I'm wondering if we should have a convenience macro in "virdomainlist.h" that covers the flags supported by default by the helper function, and where a domain could then just do: VIR_CONNECT_LIST_DOMAINS_DEFAULT | domain-specific extras to shorten this virCheckFlags.
@@ -3907,6 +3939,7 @@ static virDriver libxlDriver = { .domainEventRegisterAny = libxlDomainEventRegisterAny, /* 0.9.0 */ .domainEventDeregisterAny = libxlDomainEventDeregisterAny, /* 0.9.0 */ .isAlive = libxlIsAlive, /* 0.9.8 */ + .listAllDomains = libxlListAllDomains, /* 0.9.13 */
I'd list this line next to libxlNumOfDomains, to match the layout in driver.h. Not strictly necessary (thanks to C99 initialization syntax allowing reordering), but makes it easier to correlate to see which functions a driver has implemented if they appear in the same order. Similar comments to the other drivers.
+static int +qemuListAllDomainsManagedSaveFilter(virDomainPtr domain) +{ + struct qemud_driver *driver = domain->conn->privateData; + int ret; + + /* driver is already locked */ + virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, domain->uuid); + + ret = qemuDomainObjHasManagedSaveImage(driver, vm); + virDomainObjUnlock(vm); + return ret;
Not sure we need this function, if you add my proposed prereq patch.
+ qemuDriverLock(driver); + + if ((ret = virDomainList(conn, driver->domains.objs, domains, flags)) < 0) + goto cleanup; + + /* driver specific filters */
and you probably don't need this portion of the function, either. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The virtual box doesn't use the common virDomainObj implementation so this patch adds a separate implementation using the virtual box API. This driver implementation supports only the following filter flags: VIR_CONNECT_LIST_DOMAINS_ACTIVE VIR_CONNECT_LIST_DOMAINS_INACTIVE VIR_CONNECT_LIST_DOMAINS_TRANSIENT VIR_CONNECT_LIST_DOMAINS_PERSISTENT The latter two of these are irelevant as virtual box only supports persistent domains, so specifying only VIR_CONNECT_LIST_DOMAINS_TRANSIENT results into a empty list. --- New in series. Tested to work. --- src/vbox/vbox_tmpl.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 133 insertions(+), 0 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 4b0ee2e..36b52a8 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -9097,6 +9097,138 @@ endjob: } #endif /* VBOX_API_VERSION >= 4000 */ +static int +vboxListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + VBOX_OBJECT_CHECK(conn, int, -1); + vboxArray machines = VBOX_ARRAY_INITIALIZER; + char *machineNameUtf8 = NULL; + PRUnichar *machineNameUtf16 = NULL; + unsigned char uuid[VIR_UUID_BUFLEN]; + vboxIID iid = VBOX_IID_INITIALIZER; + PRUint32 state; + nsresult rc; + int i; + virDomainPtr dom; + virDomainPtr *doms = NULL; + size_t ndoms = 0; + int count = 0; + bool active; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT, -1); + + + rc = vboxArrayGet(&machines, data->vboxObj, data->vboxObj->vtbl->GetMachines); + if (NS_FAILED(rc)) { + vboxError(VIR_ERR_INTERNAL_ERROR, + _("Could not get list of domains, rc=%08x"), (unsigned)rc); + goto cleanup; + } + + if (domains) { + if (VIR_ALLOC_N(doms, 1) < 0) + goto no_memory; + ndoms = 1; + } + + for (i = 0; i < machines.count; i++) { + IMachine *machine = machines.items[i]; + + if (machine) { + PRBool isAccessible = PR_FALSE; + machine->vtbl->GetAccessible(machine, &isAccessible); + if (isAccessible) { + machine->vtbl->GetState(machine, &state); + + if (state >= MachineState_FirstOnline && + state <= MachineState_LastOnline) + active = true; + else + active = false; + + /* filter by active state */ + if (!(flags & VIR_CONNECT_LIST_DOMAINS_ACTIVE && + flags & VIR_CONNECT_LIST_DOMAINS_INACTIVE)) { + if (flags & VIR_CONNECT_LIST_DOMAINS_ACTIVE && + !active) + continue; + if (flags & VIR_CONNECT_LIST_DOMAINS_INACTIVE && + active) + continue; + } + + /* filter by persistent state - all vbox domains are persistent */ + if (flags & VIR_CONNECT_LIST_DOMAINS_TRANSIENT && + !(flags & VIR_CONNECT_LIST_DOMAINS_PERSISTENT)) + continue; + + /* just count the machines */ + if (!doms) { + count++; + continue; + } + + machine->vtbl->GetName(machine, &machineNameUtf16); + VBOX_UTF16_TO_UTF8(machineNameUtf16, &machineNameUtf8); + machine->vtbl->GetId(machine, &iid.value); + vboxIIDToUUID(&iid, uuid); + vboxIIDUnalloc(&iid); + + dom = virGetDomain(conn, machineNameUtf8, uuid); + + if (machineNameUtf8) { + VBOX_UTF8_FREE(machineNameUtf8); + machineNameUtf8 = NULL; + } + + if (machineNameUtf16) { + VBOX_COM_UNALLOC_MEM(machineNameUtf16); + machineNameUtf16 = NULL; + } + + if (!dom) + goto no_memory; + + if (active) + dom->id = i + 1; + + if (VIR_EXPAND_N(doms, ndoms, 1) < 0) + goto no_memory; + + doms[count++] = dom; + } + } + } + + if (doms) + *domains = doms; + doms = NULL; + ret = count; + +cleanup: + if (doms) { + for (i = 0; i < count; i++) { + if (doms[i]) + virDomainFree(doms[i]); + } + } + VIR_FREE(doms); + + vboxArrayRelease(&machines); + return ret; + +no_memory: + virReportOOMError(); + goto cleanup; +} + + + /** * Function Tables */ @@ -9175,6 +9307,7 @@ virDriver NAME(Driver) = { .domainRevertToSnapshot = vboxDomainRevertToSnapshot, /* 0.8.0 */ .domainSnapshotDelete = vboxDomainSnapshotDelete, /* 0.8.0 */ .isAlive = vboxIsAlive, /* 0.9.8 */ + .listAllDomains = vboxListAllDomains, /* 0.9.13 */ }; virNetworkDriver NAME(NetworkDriver) = { -- 1.7.3.4

On 06/05/2012 07:19 AM, Peter Krempa wrote:
The virtual box doesn't use the common virDomainObj implementation so this patch adds a separate implementation using the virtual box API.
This driver implementation supports only the following filter flags: VIR_CONNECT_LIST_DOMAINS_ACTIVE VIR_CONNECT_LIST_DOMAINS_INACTIVE VIR_CONNECT_LIST_DOMAINS_TRANSIENT VIR_CONNECT_LIST_DOMAINS_PERSISTENT The latter two of these are irelevant as virtual box only supports
s/irelevant/irrelevant/
persistent domains, so specifying only VIR_CONNECT_LIST_DOMAINS_TRANSIENT results into a empty list.
We should also be able to support _RUNNING, _PAUSED, _SHUTDOWN, and _OTHER (see vboxDomainGetInfo for how) as well as _HAS_SNAPSHOT/_NO_SNAPSHOT (see vboxDomainSnapshotNum for how).
+ + rc = vboxArrayGet(&machines, data->vboxObj, data->vboxObj->vtbl->GetMachines); + if (NS_FAILED(rc)) { + vboxError(VIR_ERR_INTERNAL_ERROR, + _("Could not get list of domains, rc=%08x"), (unsigned)rc); + goto cleanup; + } + + if (domains) { + if (VIR_ALLOC_N(doms, 1) < 0)
Instead of allocating this with 1, then reallocating down the road, I'd suggest pre-allocating with machines.count+1, then only trimming at the end if filtering took place. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Hyperv doesn't use the common virDomainObjimplementation so this patch adds a separate implementation. This driver implementation supports only the following filter flags: VIR_CONNECT_LIST_DOMAINS_ACTIVE VIR_CONNECT_LIST_DOMAINS_INACTIVE VIR_CONNECT_LIST_DOMAINS_TRANSIENT VIR_CONNECT_LIST_DOMAINS_PERSISTENT The latter two of these are irelevant as Hyperv only supports persistent domains, so specifying only VIR_CONNECT_LIST_DOMAINS_TRANSIENT results into an empty list. --- New in series. UNTESTED!! (I don't have access to Hyperv, and couldn't even get dependencies to compile this driver, so I'm not even sure if this compiles.) --- src/hyperv/hyperv_driver.c | 95 ++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 95 insertions(+), 0 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 3b15292..bf5848f 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1259,6 +1259,100 @@ hypervDomainManagedSaveRemove(virDomainPtr domain, unsigned int flags) } +static int +hypervListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + hypervPrivate *priv = conn->privateData; + virBuffer query = VIR_BUFFER_INITIALIZER; + Msvm_ComputerSystem *computerSystemList = NULL; + Msvm_ComputerSystem *computerSystem = NULL; + size_t ndoms; + virDomainPtr domain; + virDomainPtr *doms = NULL; + int count = 0; + int ret = -1; + int i; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT, -1); + + virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); + virBufferAddLit(&query, "where "); + virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); + + /* construct query with filter depending on flags */ + if (!(flags & VIR_CONNECT_LIST_DOMAINS_ACTIVE && + flags & VIR_CONNECT_LIST_DOMAINS_INACTIVE)) { + if (flags & VIR_CONNECT_LIST_DOMAINS_ACTIVE) { + virBufferAddLit(&query, "and "); + virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_ACTIVE); + } + if (flags & VIR_CONNECT_LIST_DOMAINS_INACTIVE) { + virBufferAddLit(&query, "and "); + virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_INACTIVE); + } + } + + /* filter by persistent state - all vbox domains are persistent */ + if (flags & VIR_CONNECT_LIST_DOMAINS_TRANSIENT && + !(flags & VIR_CONNECT_LIST_DOMAINS_PERSISTENT)) + goto cleanup; + + if (hypervGetMsvmComputerSystemList(priv, &query, + &computerSystemList) < 0) + goto cleanup; + + if (domains) { + if (VIR_ALLOC_N(doms, 1) < 0) + goto no_memory; + ndoms = 1; + } + + for (computerSystem = computerSystemList; computerSystem != NULL; + computerSystem = computerSystem->next) { + + if (!doms) { + count++; + continue; + } + + if (hypervMsvmComputerSystemToDomain(conn, computerSystem, + &domain) < 0) + goto cleanup; + + if (VIR_EXPAND_N(doms, ndoms, 1) < 0) + goto no_memory; + doms[count++] = domain; + } + + if (doms) + *domains = doms; + doms = NULL; + ret = count; + +cleanup: + if (doms) { + for (i = 0; i < count; ++i) { + if (doms[i]) + virDomainFree(doms[i]); + } + } + + VIR_FREE(doms); + hypervFreeObject(priv, (hypervObject *)computerSystemList); + return ret; + +no_memory: + virReportOOMError(); + goto cleanup; +} + + + static virDriver hypervDriver = { .no = VIR_DRV_HYPERV, @@ -1294,6 +1388,7 @@ static virDriver hypervDriver = { .domainHasManagedSaveImage = hypervDomainHasManagedSaveImage, /* 0.9.5 */ .domainManagedSaveRemove = hypervDomainManagedSaveRemove, /* 0.9.5 */ .isAlive = hypervIsAlive, /* 0.9.8 */ + .listAllDomains = hypervListAllDomains, /* 0.9.13 */ }; -- 1.7.3.4

On 06/05/2012 07:19 AM, Peter Krempa wrote:
Hyperv doesn't use the common virDomainObjimplementation so this patch adds a separate implementation.
This driver implementation supports only the following filter flags: VIR_CONNECT_LIST_DOMAINS_ACTIVE VIR_CONNECT_LIST_DOMAINS_INACTIVE VIR_CONNECT_LIST_DOMAINS_TRANSIENT VIR_CONNECT_LIST_DOMAINS_PERSISTENT The latter two of these are irelevant as Hyperv only supports persistent
s/irelevant/irrelevant/
domains, so specifying only VIR_CONNECT_LIST_DOMAINS_TRANSIENT results into an empty list.
It looks like hypervDomainGetInfo uses a helper hypervMsvmComputerSystemEnabledStateToDomainState to get domain state, so you should also be able to support _RUNNING, _PAUSED, _SHUTOFF, and _OTHER - and I'd really like to guarantee that much for all domains that implement this API. Also, since managedsave is present (hypervDomainHasManagedSaveImage), you should definitely support [NO]_MANAGEDSAVE. You could also trivially support [HAS|NO]_SNAPSHOT in the same way that you trivially support _PERSISTENT by always returning 0 for that one, but the API documentation will let us go either way.
--- New in series. UNTESTED!! (I don't have access to Hyperv, and couldn't even get dependencies to compile this driver, so I'm not even sure if this compiles.)
I don't have access to test hyperv, but I do have the dependencies for compilation (but don't ask me to remember how I got it installed), and can report: Compilation succeeded!
+static int +hypervListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + hypervPrivate *priv = conn->privateData; + virBuffer query = VIR_BUFFER_INITIALIZER; + Msvm_ComputerSystem *computerSystemList = NULL; + Msvm_ComputerSystem *computerSystem = NULL; + size_t ndoms; + virDomainPtr domain; + virDomainPtr *doms = NULL; + int count = 0; + int ret = -1; + int i;
Based on comparison with other code, what you have seems reasonable, but incomplete given what else I think is possible by copy-and-paste.
+ if (domains) { + if (VIR_ALLOC_N(doms, 1) < 0) + goto no_memory; + ndoms = 1; + } + + for (computerSystem = computerSystemList; computerSystem != NULL; + computerSystem = computerSystem->next) { + + if (!doms) { + count++; + continue;
Unlike my complaints in 5/9 and 6/9, here, you really are best doing a reallocation during the iteration, since you don't know up front how long the list is. However, I also think that you can use VIR_RESIZE_N along with an extra allocation tracking variable for geometric growth and less overhead than quadratic effects from reallocation on every iteration.
static virDriver hypervDriver = { .no = VIR_DRV_HYPERV, @@ -1294,6 +1388,7 @@ static virDriver hypervDriver = { .domainHasManagedSaveImage = hypervDomainHasManagedSaveImage, /* 0.9.5 */ .domainManagedSaveRemove = hypervDomainManagedSaveRemove, /* 0.9.5 */ .isAlive = hypervIsAlive, /* 0.9.8 */ + .listAllDomains = hypervListAllDomains, /* 0.9.13 */
As with other patches in the series, I'd consider listing this in the order where it appears in driver.h, although it is not a show-stopper. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Esx doesn't use the common virDomainObjimplementation so this patch adds a separate implementation. This driver implementation supports only the following filter flags: VIR_CONNECT_LIST_DOMAINS_ACTIVE VIR_CONNECT_LIST_DOMAINS_INACTIVE VIR_CONNECT_LIST_DOMAINS_TRANSIENT VIR_CONNECT_LIST_DOMAINS_PERSISTENT The latter two of these are irelevant as Esx only supports persistent domains, so specifying only VIR_CONNECT_LIST_DOMAINS_TRANSIENT results into an empty list. --- New in series. UNTESTED!!! (I don't have access to esx, compiles) --- src/esx/esx_driver.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 108 insertions(+), 0 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index b3f1948..ba47326 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4948,6 +4948,113 @@ esxDomainGetMemoryParameters(virDomainPtr domain, virTypedParameterPtr params, } +static int +esxListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + int ret = -1; + esxPrivate *priv = conn->privateData; + virDomainPtr dom; + virDomainPtr *doms = NULL; + size_t ndoms = 0; + esxVI_ObjectContent *virtualMachineList = NULL; + esxVI_ObjectContent *virtualMachine = NULL; + esxVI_String *propertyNameList = NULL; + esxVI_VirtualMachinePowerState powerState; + char *name = NULL; + int id; + unsigned char uuid[VIR_UUID_BUFLEN]; + int count = 0; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT, -1); + + if (esxVI_EnsureSession(priv->primary) < 0) + return -1; + + if (esxVI_String_AppendValueToList(&propertyNameList, + "runtime.powerState") < 0 || + esxVI_LookupVirtualMachineList(priv->primary, propertyNameList, + &virtualMachineList) < 0) + goto cleanup; + + if (domains) { + if (VIR_ALLOC_N(doms, 1) < 0) + goto no_memory; + ndoms = 1; + } + + for (virtualMachine = virtualMachineList; virtualMachine != NULL; + virtualMachine = virtualMachine->_next) { + + VIR_FREE(name); + + if (esxVI_GetVirtualMachineIdentity(virtualMachine, &id, &name, uuid) < 0 || + esxVI_GetVirtualMachinePowerState(virtualMachine, &powerState) < 0) + goto cleanup; + + /* filter by state */ + if (!(flags & VIR_CONNECT_LIST_DOMAINS_ACTIVE && + flags & VIR_CONNECT_LIST_DOMAINS_INACTIVE)) { + if (flags & VIR_CONNECT_LIST_DOMAINS_ACTIVE && + powerState == esxVI_VirtualMachinePowerState_PoweredOff) + continue; + if (flags & VIR_CONNECT_LIST_DOMAINS_INACTIVE && + powerState != esxVI_VirtualMachinePowerState_PoweredOff) + continue; + } + + /* filter by persistent state - all vbox domains are persistent */ + if (flags & VIR_CONNECT_LIST_DOMAINS_TRANSIENT && + !(flags & VIR_CONNECT_LIST_DOMAINS_PERSISTENT)) + continue; + + /* just count the machines */ + if (!doms) { + count++; + continue; + } + + if (!(dom = virGetDomain(conn, name, uuid))) + goto no_memory; + + /* Only running/suspended virtual machines have an ID != -1 */ + if (powerState != esxVI_VirtualMachinePowerState_PoweredOff) + dom->id = id; + else + dom->id = -1; + + if (VIR_EXPAND_N(doms, ndoms, 1) < 0) + goto no_memory; + doms[count++] = dom; + } + + if (doms) + *domains = doms; + doms = NULL; + ret = count; + +cleanup: + if (doms) { + for (id = 0; id < count; id++) { + if (doms[id]) + virDomainFree(doms[id]); + } + } + VIR_FREE(doms); + VIR_FREE(name); + esxVI_String_Free(&propertyNameList); + esxVI_ObjectContent_Free(&virtualMachineList); + return ret; + +no_memory: + virReportOOMError(); + goto cleanup; +} + static virDriver esxDriver = { .no = VIR_DRV_ESX, @@ -5023,6 +5130,7 @@ static virDriver esxDriver = { .domainRevertToSnapshot = esxDomainRevertToSnapshot, /* 0.8.0 */ .domainSnapshotDelete = esxDomainSnapshotDelete, /* 0.8.0 */ .isAlive = esxIsAlive, /* 0.9.8 */ + .listAllDomains = esxListAllDomains, /* 0.9.13 */ }; -- 1.7.3.4

On 06/05/2012 07:19 AM, Peter Krempa wrote:
Esx doesn't use the common virDomainObjimplementation so
s/Objimp/Obj imp/ (Hmm, I think you had the same typo in 7/9)
this patch adds a separate implementation.
This driver implementation supports only the following filter flags: VIR_CONNECT_LIST_DOMAINS_ACTIVE VIR_CONNECT_LIST_DOMAINS_INACTIVE VIR_CONNECT_LIST_DOMAINS_TRANSIENT VIR_CONNECT_LIST_DOMAINS_PERSISTENT The latter two of these are irelevant as Esx only supports persistent
s/irelevant/irrelevant/
domains, so specifying only VIR_CONNECT_LIST_DOMAINS_TRANSIENT results into an empty list.
ESX supports domain states (_RUNNING, ...; see esxVI_VirtualMachinePowerState_ConvertToLibvirt as used in esxDomainGetInfo), autostart (esxDomainGetAutostart), and snapshots (esxDomainSnapshotNum), so we should definitely be supporting those flags. Also, since there is no managedsave, we could trivially implement that the same way we implement _TRANSIENT.
--- New in series. UNTESTED!!! (I don't have access to esx, compiles)
I do, and it compiled (but like hyperv, I didn't test the result). At this point, it might be wise to respin the series, and push the individual driver pieces as and when they get reviews from someone that is actually able to test them.
+ + if (VIR_EXPAND_N(doms, ndoms, 1) < 0) + goto no_memory; + doms[count++] = dom;
Same story about considering VIR_RESIZE_N. Looking forward to v3 of this series. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch makes use of the newly added api virConnectListAllDomains() to list domains in virsh. To enable fallback 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 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 of some of the vlags supported by virConnectListAllDomains(). This patch also cleans up the "list" command handler to use the new helpers. --- Diff to v1: - moved filtering to the helper function - cleaned up flag handling in cmdList --- tools/virsh.c | 469 ++++++++++++++++++++++++++++++++++++--------------------- 1 files changed, 297 insertions(+), 172 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index abcfbff..f258f59 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -64,6 +64,7 @@ #include "util/bitmap.h" #include "conf/domain_conf.h" #include "virtypedparam.h" +#include "intprops.h" static char *progname; @@ -490,22 +491,252 @@ _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) +{ + const char **sa = (const char**)a; + const char **sb = (const char**)b; - if (*ia > *ib) - return 1; - else if (*ia < *ib) - return -1; - return 0; + /* User visible sort, so we want locale-specific case comparison. */ + return strcasecmp(*sa, *sb); +} + +static int +domsorter(const void *a, const void *b) +{ + virDomainPtr *da = (virDomainPtr *) a; + virDomainPtr *db = (virDomainPtr *) b; + unsigned int ida = virDomainGetID(*da); + unsigned int idb = virDomainGetID(*db); + unsigned int inactive = (unsigned int) -1; + + if (ida == inactive && idb == inactive) + return strcasecmp(virDomainGetName(*da), virDomainGetName(*db)); + + if (ida != inactive && idb != inactive) { + if (ida > idb) + return 1; + else if (ida < idb) + return -1; + } + + if (ida != inactive) + return -1; + else + return 1; +} + +struct vshDomainList { + virDomainPtr *domains; + int ndomains; +}; +typedef struct vshDomainList *vshDomainListPtr; + +/* return 0 if false, 1 if true -1 if error */ +typedef int (*vshDomainListFilter)(vshControl *ctl, virDomainPtr dom); + +/* collection of filters */ +static int +vshDomListPersistFilter(vshControl *ctl, virDomainPtr dom) +{ + int persistent = virDomainIsPersistent(dom); + if (persistent < 0) + vshError(ctl, "%s", _("Failed to determine domain's persistent state")); + return persistent; +} + +static int +vshDomListFilter(vshControl *ctl, vshDomainListPtr domlist, + vshDomainListFilter filter, bool expect_result) +{ + int i = 0; + int ret; + + while (i < domlist->ndomains) { + ret = filter(ctl, domlist->domains[i]); + if (ret < 0) + return -1; + + if (!ret == !expect_result) { + virDomainFree(domlist->domains[i]); + if (i != --domlist->ndomains) + memmove(&domlist->domains[i], + &domlist->domains[i+1], + sizeof(*domlist->domains) * (domlist->ndomains - i)); + + if (VIR_REALLOC_N(domlist->domains, domlist->ndomains) < 0) { + ; /* not fatal */ + } + continue; + } + i++; + } + return 0; +} + +static int +vshDomList(vshControl *ctl, vshDomainListPtr domlist, unsigned int flags) +{ + int rv = -1; + virErrorPtr err = NULL; + int i; + int *ids = NULL; + int nids = 0; + char **names = NULL; + int nnames = 0; + virDomainPtr dom; + virDomainPtr *doms = NULL; + int ndoms = 0; + + domlist->domains = NULL; + domlist->ndomains = 0; + + /* try the list with flags support */ + if ((ndoms = virConnectListAllDomains(ctl->conn, &doms, flags)) >= 0) { + domlist->ndomains = ndoms; + domlist->domains = doms; + doms = NULL; + goto finished; + } + + /* check if the command is actualy supported or it was an error */ + if ((err = virGetLastError()) && + !(err->code == VIR_ERR_NO_SUPPORT || + err->code == VIR_ERR_INVALID_ARG)) { + vshError(ctl, "%s", _("Failed to list domains")); + goto cleanup; + } + + /* fall back to old method */ + virResetLastError(); + + /* list active domains, if necessary */ + if (!(flags & + (VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE)) || + flags & VIR_CONNECT_LIST_DOMAINS_ACTIVE) { + + if ((nids = virConnectNumOfDomains(ctl->conn)) < 0) { + vshError(ctl, "%s", _("Failed to list active domains")); + goto cleanup; + } + + if (nids) { + ids = vshMalloc(ctl, sizeof(int) * nids); + + if ((nids = virConnectListDomains(ctl->conn, ids, nids)) < 0) { + vshError(ctl, "%s", _("Failed to list active domains")); + goto cleanup; + } + } + } + + if (!(flags & + (VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE)) || + flags & VIR_CONNECT_LIST_DOMAINS_INACTIVE) { + + if ((nnames = virConnectNumOfDefinedDomains(ctl->conn)) < 0) { + vshError(ctl, "%s", _("Failed to list inactive domains")); + goto cleanup; + } + + if (nnames) { + names = vshMalloc(ctl, sizeof(char *) * nnames); + + if ((nnames = virConnectListDefinedDomains(ctl->conn, names, + nnames)) < 0) { + vshError(ctl, "%s", _("Failed to list inactive domains")); + goto cleanup; + } + } + } + + doms = vshMalloc(ctl, sizeof(virDomainPtr) * (nids + nnames)); + ndoms = 0; + + /* get active domains */ + for (i = 0; i < nids; i++) { + if (!(dom = virDomainLookupByID(ctl->conn, ids[i]))) + continue; + doms[ndoms++] = dom; + } + + /* get inctive domains */ + for (i = 0; i < nnames; i++) { + if (!(dom = virDomainLookupByName(ctl->conn, names[i]))) + continue; + doms[ndoms++] = dom; + } + + /* filter list the list if the list was acquired by fallback means */ + domlist->domains = doms; + domlist->ndomains = ndoms; + doms = NULL; + + /* filter by active state */ + /* already done while getting the list by fallback means */ + + /* filter by persistent state */ + if (!(flags & VIR_CONNECT_LIST_DOMAINS_PERSISTENT && + flags & VIR_CONNECT_LIST_DOMAINS_TRANSIENT)) { + /* remove transient domains */ + if (flags & VIR_CONNECT_LIST_DOMAINS_PERSISTENT && + vshDomListFilter(ctl, domlist, vshDomListPersistFilter, false) < 0) + goto cleanup; + + /* remove persistent domains */ + if (flags & VIR_CONNECT_LIST_DOMAINS_TRANSIENT && + vshDomListFilter(ctl, domlist, vshDomListPersistFilter, true) < 0) + goto cleanup; + } + + /* reject all other filters - they are not needed by now */ + if (flags & + ~(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT)) { + vshError(ctl, "%s", _("Filter flags unsupported in fallback filter")); + goto cleanup; + } + +finished: + qsort(domlist->domains, domlist->ndomains, + sizeof(*domlist->domains), domsorter); + + rv = ndoms; + +cleanup: + for (i = 0; i < nnames; i++) + VIR_FREE(names[i]); + if (doms) { + for (i = 0; i < ndoms; i++) { + if (doms[i]) + virDomainFree(doms[i]); + } + } + VIR_FREE(doms); + VIR_FREE(names); + VIR_FREE(ids); + return rv; } -static int namesorter(const void *a, const void *b) { - const char **sa = (const char**)a; - const char **sb = (const char**)b; - /* User visible sort, so we want locale-specific case comparison. */ - return strcasecmp(*sa, *sb); +static void +vshDomListFree(vshDomainListPtr domlist) +{ + int i; + + if (!domlist || !domlist->domains) + return; + + for (i = 0; i < domlist->ndomains; i++) { + if (domlist->domains[i]) + virDomainFree(domlist->domains[i]); + } + + domlist->ndomains = 0; + VIR_FREE(domlist->domains); } static double @@ -990,35 +1221,32 @@ static const vshCmdOptDef opts_list[] = { static bool cmdList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { - bool inactive = vshCommandOptBool(cmd, "inactive"); - bool all = vshCommandOptBool(cmd, "all"); - bool active = !inactive || all; - int *ids = NULL, maxid = 0, i; - char **names = NULL; - int maxname = 0; bool managed = vshCommandOptBool(cmd, "managed-save"); bool optTitle = vshCommandOptBool(cmd, "title"); bool optTable = vshCommandOptBool(cmd, "table"); bool optUUID = vshCommandOptBool(cmd, "uuid"); bool optName = vshCommandOptBool(cmd, "name"); - bool optPersistent = vshCommandOptBool(cmd, "persistent"); - bool optTransient = vshCommandOptBool(cmd, "transient"); - bool persistUsed = true; - virDomainPtr dom = NULL; + int i; char *title; char uuid[VIR_UUID_STRING_BUFLEN]; int state; - int persistent; bool ret = false; + struct vshDomainList list; + char id[INT_BUFSIZE_BOUND(unsigned int)]; + unsigned int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE; - inactive |= all; + if (vshCommandOptBool(cmd, "inactive")) + flags = VIR_CONNECT_LIST_DOMAINS_INACTIVE; - /* process arguments */ - if (!optPersistent && !optTransient) { - optPersistent = true; - optTransient = true; - persistUsed = false; - } + if (vshCommandOptBool(cmd, "all")) + flags = VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_ACTIVE; + + if (vshCommandOptBool(cmd, "persistent")) + flags |= VIR_CONNECT_LIST_DOMAINS_PERSISTENT; + + if (vshCommandOptBool(cmd, "transient")) + flags |= VIR_CONNECT_LIST_DOMAINS_TRANSIENT; if (optTable + optName + optUUID > 1) { vshError(ctl, "%s", @@ -1033,167 +1261,64 @@ cmdList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (active) { - maxid = virConnectNumOfDomains(ctl->conn); - if (maxid < 0) { - vshError(ctl, "%s", _("Failed to list active domains")); - return false; - } - if (maxid) { - ids = vshMalloc(ctl, sizeof(int) * maxid); - - if ((maxid = virConnectListDomains(ctl->conn, ids, maxid)) < 0) { - vshError(ctl, "%s", _("Failed to list active domains")); - goto cleanup; - } - - qsort(ids, maxid, sizeof(int), idsorter); - } - } - - if (inactive) { - maxname = virConnectNumOfDefinedDomains(ctl->conn); - if (maxname < 0) { - vshError(ctl, "%s", _("Failed to list inactive domains")); - goto cleanup; - } - if (maxname) { - names = vshMalloc(ctl, sizeof(char *) * maxname); - - if ((maxname = virConnectListDefinedDomains(ctl->conn, - names, - maxname)) < 0) { - vshError(ctl, "%s", _("Failed to list inactive domains")); - goto cleanup; - } - - qsort(&names[0], maxname, sizeof(char*), namesorter); - } - } + if (vshDomList(ctl, &list, flags) < 0) + goto cleanup; /* print table header in legacy mode */ if (optTable) { - if (optTitle) { + if (optTitle) vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n", _("Id"), _("Name"), _("State"), _("Title"), "-----------------------------------------" "-----------------------------------------"); - } else { + else vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n", _("Id"), _("Name"), _("State"), "-----------------------------------------" "-----------"); - } } - for (i = 0; i < maxid; i++) { - dom = virDomainLookupByID(ctl->conn, ids[i]); - - /* this kind of work with domains is not atomic operation */ - if (!dom) - continue; + for (i = 0; i < list.ndomains; i++) { + if (virDomainGetID(list.domains[i]) != (unsigned int) -1) + snprintf(id, sizeof(id), "%d", virDomainGetID(list.domains[i])); + else + virStrncpy(id, "-", sizeof(id)); - if (persistUsed) { - persistent = virDomainIsPersistent(dom); - if (persistent < 0) { - vshError(ctl, "%s", - _("Failed to determine domain's persistent state")); - goto cleanup; - } + state = vshDomainState(ctl, list.domains[i], NULL); + if (optTable && managed && state == VIR_DOMAIN_SHUTOFF && + virDomainHasManagedSaveImage(list.domains[i], 0) > 0) + state = -2; - if (!(persistent ? optPersistent : optTransient)) { - virDomainFree(dom); - dom = NULL; - continue; - } - } - - if (optTable) { - if (optTitle) { - if (!(title = vshGetDomainDescription(ctl, dom, true, 0))) - goto cleanup; - - vshPrint(ctl, " %-5d %-30s %-10s %-20s\n", - virDomainGetID(dom), - virDomainGetName(dom), - _(vshDomainStateToString(vshDomainState(ctl, dom, NULL))), - title); - VIR_FREE(title); - } else { - vshPrint(ctl, " %-5d %-30s %s\n", - virDomainGetID(dom), - virDomainGetName(dom), - _(vshDomainStateToString(vshDomainState(ctl, dom, NULL)))); - } - } else if (optUUID) { - if (virDomainGetUUIDString(dom, uuid) < 0) { - vshError(ctl, "%s", - _("Failed to get domain's UUID")); - goto cleanup; - } - vshPrint(ctl, "%s\n", uuid); - } else if (optName) { - vshPrint(ctl, "%s\n", virDomainGetName(dom)); - } - - virDomainFree(dom); - dom = NULL; - } - - if (optPersistent) { - for (i = 0; i < maxname; i++) { - dom = virDomainLookupByName(ctl->conn, names[i]); - - /* this kind of work with domains is not atomic operation */ - if (!dom) - continue; + if (optTable) { + if (optTitle) { + if (!(title = vshGetDomainDescription(ctl, list.domains[i], true, 0))) + goto cleanup; - if (optTable) { - state = vshDomainState(ctl, dom, NULL); - if (managed && state == VIR_DOMAIN_SHUTOFF && - virDomainHasManagedSaveImage(dom, 0) > 0) - state = -2; - - if (optTitle) { - if (!(title = vshGetDomainDescription(ctl, dom, true, 0))) - goto cleanup; - - vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", - "-", - names[i], - state == -2 ? _("saved") : _(vshDomainStateToString(state)), - title); - VIR_FREE(title); - } else { - vshPrint(ctl, " %-5s %-30s %s\n", - "-", - names[i], - state == -2 ? _("saved") : _(vshDomainStateToString(state))); - } - } else if (optUUID) { - if (virDomainGetUUIDString(dom, uuid) < 0) { - vshError(ctl, "%s", - _("Failed to get domain's UUID")); - goto cleanup; - } - vshPrint(ctl, "%s\n", uuid); - } else if (optName) { - vshPrint(ctl, "%s\n", names[i]); - } + vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id, + virDomainGetName(list.domains[i]), + state == -2 ? _("saved") : _(vshDomainStateToString(state)), + title); - virDomainFree(dom); - dom = NULL; + VIR_FREE(title); + } else { + vshPrint(ctl, " %-5s %-30s %s\n", id, + virDomainGetName(list.domains[i]), + state == -2 ? _("saved") : _(vshDomainStateToString(state))); + } + } else if (optUUID) { + if (virDomainGetUUIDString(list.domains[i], uuid) < 0) { + vshError(ctl, "%s", _("Failed to get domain's UUID")); + goto cleanup; + } + vshPrint(ctl, "%s\n", uuid); + } else if (optName) { + vshPrint(ctl, "%s\n", virDomainGetName(list.domains[i])); } } ret = true; cleanup: - if (dom) - virDomainFree(dom); - VIR_FREE(ids); - for (i = 0; i < maxname; i++) - VIR_FREE(names[i]); - VIR_FREE(names); + vshDomListFree(&list); return ret; } -- 1.7.3.4

On 06/05/2012 07:19 AM, Peter Krempa wrote:
This patch makes use of the newly added api virConnectListAllDomains() to list domains in virsh.
I would rebase this patch to come right after 1/9 but before any driver implementation. Why? Because then you will have automatic testing that the fallback code actually works, prior to implementing the new API in later patches. In fact, I'm reviewing it out of order (before 5/9) so that I can test the fallback mode first.
To enable fallback 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 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 of some of the vlags supported by virConnectListAllDomains().
This patch also cleans up the "list" command handler to use the new helpers. --- Diff to v1: - moved filtering to the helper function - cleaned up flag handling in cmdList --- tools/virsh.c | 469 ++++++++++++++++++++++++++++++++++++--------------------- 1 files changed, 297 insertions(+), 172 deletions(-)
Question - should we be adding new flags to virsh.c for 'virsh list', and therefore new documentation to virsh.pod, to expose the new filtering bits? But that would be okay as a followup patch. I've got a pending patch series in my sandbox for virDomainListAllSnapshots, where I split things into adding new filter arguments in virsh prior to adding the new list API (hmm, I'd better dust that work off and get it posted now, but it is based on top of this as-yet-unreviewed series, if you want to trade reviews :) https://www.redhat.com/archives/libvir-list/2012-May/msg01196.html I don't know what happened, but this doesn't compile (probably a mistake on your part in a last-minute silencing of a 'make syntax-check' failure): virsh.c: In function 'cmdList': virsh.c:1285:13: error: too few arguments to function 'virStrncpy' In file included from virsh.c:53:0:
+static int +vshDomList(vshControl *ctl, vshDomainListPtr domlist, unsigned int flags) +{ + int rv = -1; + virErrorPtr err = NULL; + int i; + int *ids = NULL; + int nids = 0; + char **names = NULL; + int nnames = 0; + virDomainPtr dom; + virDomainPtr *doms = NULL; + int ndoms = 0; + + domlist->domains = NULL; + domlist->ndomains = 0; + + /* try the list with flags support */ + if ((ndoms = virConnectListAllDomains(ctl->conn, &doms, flags)) >= 0) { + domlist->ndomains = ndoms; + domlist->domains = doms; + doms = NULL; + goto finished; + }
So much easier when we are done here :)
+ + /* check if the command is actualy supported or it was an error */
s/actualy/actually/
+ if ((err = virGetLastError()) && + !(err->code == VIR_ERR_NO_SUPPORT || + err->code == VIR_ERR_INVALID_ARG)) { + vshError(ctl, "%s", _("Failed to list domains")); + goto cleanup; + }
Careful. VIR_ERR_NO_SUPPORT means that virConnectListAllDomains() was not supported, so we have to use the old method to construct the list. But VIR_ERR_INVALID_ARG means that virConnectListAllDomains() _is_ supported, just that it doesn't support the full range of flags according to what we passed in. In that case, what we should do is call virConnectListAllDomains(ctl->conn, &doms, 0), then we can 'goto filters' where we use the same code to run filters as the old method (at least we grabbed the list race-free). [1] Hmm, I think we need to guarantee in the API documentation in 1/9 that all drivers are guaranteed to support _ACTIVE, _INACTIVE, _PERSISTENT, _TRANSIENT, and the four state flags; and only permit failures on the remaining 3 groups (autostart, snapshot, managedsave), since only those latter features are optional for a given driver (and since we might add future optional groups). Then it would be a case of: virConnectListAllDomains(ctl->conn, &doms, flags & (_ACTIVE|_INACTIVE)) rather than 0, before jumping to the filter label.
+ + /* 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; + } + + /* filter list the list if the list was acquired by fallback means */
[1] Need the 'filter' label here.
+ domlist->domains = doms; + domlist->ndomains = ndoms; + doms = NULL; + + /* filter by active state */ + /* already done while getting the list by fallback means */ + + /* filter by persistent state */ + if (!(flags & VIR_CONNECT_LIST_DOMAINS_PERSISTENT && + flags & VIR_CONNECT_LIST_DOMAINS_TRANSIENT)) { + /* remove transient domains */ + if (flags & VIR_CONNECT_LIST_DOMAINS_PERSISTENT && + vshDomListFilter(ctl, domlist, vshDomListPersistFilter, false) < 0) + goto cleanup; + + /* remove persistent domains */ + if (flags & VIR_CONNECT_LIST_DOMAINS_TRANSIENT && + vshDomListFilter(ctl, domlist, vshDomListPersistFilter, true) < 0) + goto cleanup; + } + + /* reject all other filters - they are not needed by now */ + if (flags & + ~(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_TRANSIENT)) { + vshError(ctl, "%s", _("Filter flags unsupported in fallback filter")); + goto cleanup; + }
The goal of virsh is to expose all the flags of the underlying API, so we should eventually be supporting all the flags, even if we have to error out when we get to the fallback paths.
+ for (i = 0; i < list.ndomains; i++) { + if (virDomainGetID(list.domains[i]) != (unsigned int) -1) + snprintf(id, sizeof(id), "%d", virDomainGetID(list.domains[i])); + else + virStrncpy(id, "-", sizeof(id));
This was the compile failure. Once I fixed it to virStrcpyStatic(id, "-"), my smoke testing of the fallback mode appeared to do the right things. Overall, I like the way this is headed. The diffstat lies - we may have added more lines than we removed, but the end result is that vshCmdList is much simpler to read and we now have a reusable listing mechanism for anything else in this file (or even as an example for other applications to borrow from in using the same fallback methods). I do think a v3 would be worthwhile, though, especially if you add new flags to 'virsh list'. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 06/05/2012 05:07 PM, Eric Blake wrote:
On 06/05/2012 07:19 AM, Peter Krempa wrote:
This patch makes use of the newly added api virConnectListAllDomains() to list domains in virsh.
I would rebase this patch to come right after 1/9 but before any driver implementation. Why? Because then you will have automatic testing that the fallback code actually works, prior to implementing the new API in later patches. In fact, I'm reviewing it out of order (before 5/9) so that I can test the fallback mode first.
Question - should we be adding new flags to virsh.c for 'virsh list', and therefore new documentation to virsh.pod, to expose the new filtering bits? But that would be okay as a followup patch. I've got a pending patch series in my sandbox for virDomainListAllSnapshots, where I split things into adding new filter arguments in virsh prior to adding the new list API (hmm, I'd better dust that work off and get it posted now, but it is based on top of this as-yet-unreviewed series, if you want to trade reviews :)
Here's a link to that thread, so we can take the best ideas from each other in our next round of posting. https://www.redhat.com/archives/libvir-list/2012-June/msg00284.html -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Peter Krempa