6:28 a.m.
On 02/26/2013 07:54 PM, Eric Blake wrote:
On 02/26/2013 07:20 AM, John Ferlan wrote:
I would have put '---' here, since...
The information was only in the email not part of the git history.
In the future I'll remember to put that after the '---'.
> examples/hellolibvirt/hellolibvirt.c | 26
++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> @@ -93,7 +94,7 @@ showDomains(virConnectPtr conn)
> char **nameList = NULL;
>
> numActiveDomains = virConnectNumOfDomains(conn);
> - if (-1 == numActiveDomains) {
> + if (numActiveDomains == -1) {
> ret = 1;
> printf("Failed to get number of active domains\n");
> showError(conn);
virConnectNumOfDomains is inherently racy. Wouldn't it be better to
just drop this section of our example?
Considering the back and forth I decided to make "less" change because
it's just an example, but I suppose I took too much off the top.
So below are proposed adjustments.
> @@ -101,7 +102,7 @@ showDomains(virConnectPtr conn)
> }
>
> numInactiveDomains = virConnectNumOfDefinedDomains(conn);
> - if (-1 == numInactiveDomains) {
> + if (numInactiveDomains == -1) {
> ret = 1;
> printf("Failed to get number of inactive domains\n");
> showError(conn);
Same question.
> @@ -113,17 +114,20 @@ showDomains(virConnectPtr conn)
>
> nameList = malloc(sizeof(*nameList) * numInactiveDomains);
>
> - if (NULL == nameList) {
> + if (!nameList) {
> ret = 1;
> printf("Could not allocate memory for list of inactive
domains\n");
> goto out;
> }
>
> + /* The virConnectListDomains() will return a list of the active domains.
> + * Alternatively the virConnectListAllDomains() API would return a list
> + * of both active and inactive domains */
> numNames = virConnectListDefinedDomains(conn,
> nameList,
> numInactiveDomains);
I think it would be better to update the example to JUST use
virConnectListAllDomains(), and completely avoid
virConnectListDefinedDomains.
>
> - if (-1 == numNames) {
> + if (numNames == -1) {
> ret = 1;
> printf("Could not get list of defined domains from
hypervisor\n");
> showError(conn);
> @@ -136,9 +140,7 @@ showDomains(virConnectPtr conn)
>
> for (i = 0 ; i < numNames ; i++) {
> printf(" %s\n", *(nameList + i));
> - /* The API documentation doesn't say so, but the names
> - * returned by virConnectListDefinedDomains are strdup'd and
> - * must be freed here. */
> + /* must free the returned named per the API documentation */
> free(*(nameList + i));
Pre-existing, but '*(nameList + i)' looks odd; 'nameList[i]' looks
nicer.
Here's the differences from the v3 to what seems to be a happy medium.
The virConnectNum* functions are still used just to "show" they exist
and how to use them. There's a comment before the usage.
diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellol
index 335a75e..26dd67f 100644
--- a/examples/hellolibvirt/hellolibvirt.c
+++ b/examples/hellolibvirt/hellolibvirt.c
@@ -91,8 +91,14 @@ static int
showDomains(virConnectPtr conn)
{
int ret = 0, i, numNames, numInactiveDomains, numActiveDomains;
- char **nameList = NULL;
-
+ int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+ VIR_CONNECT_LIST_DOMAINS_INACTIVE;
+ virDomainPtr *nameList = NULL;
+ * the current call. A domain could be started or stopped and any
+ * assumptions made purely on these return values could result in
+ * unexpected results */
numActiveDomains = virConnectNumOfDomains(conn);
if (numActiveDomains == -1) {
ret = 1;
@@ -112,40 +118,24 @@ showDomains(virConnectPtr conn)
printf("There are %d active and %d inactive domains\n",
numActiveDomains, numInactiveDomains);
- nameList = malloc(sizeof(*nameList) * numInactiveDomains);
-
- if (!nameList) {
- ret = 1;
- printf("Could not allocate memory for list of inactive domains\n");
- goto out;
- }
-
- /* The virConnectListDomains() will return a list of the active domains.
- * Alternatively the virConnectListAllDomains() API would return a list
- * of both active and inactive domains */
- numNames = virConnectListDefinedDomains(conn,
- nameList,
- numInactiveDomains);
-
- if (numNames == -1) {
- ret = 1;
- printf("Could not get list of defined domains from hypervisor\n");
- showError(conn);
- goto out;
- }
-
- if (numNames > 0) {
- printf("Inactive domains:\n");
- }
-
- for (i = 0 ; i < numNames ; i++) {
- printf(" %s\n", *(nameList + i));
+ /* Return a list of all active and inactive domains. Using this API
+ * instead of virConnectListDomains() and virConnectListDefinedDomains()
+ * is preferred since it "solves" an inherit race between separated API
+ * calls if domains are started or stopped between calls */
+ numNames = virConnectListAllDomains(conn,
+ &nameList,
+ flags);
+ for (i = 0; i < numNames; i++) {
+ int active = virDomainIsActive(nameList[i]);
+ printf(" %8s (%s)\n",
+ virDomainGetName(nameList[i]),
+ (active == 1 ? "active" : "non-active"));
/* must free the returned named per the API documentation */
- free(*(nameList + i));
+ virDomainFree(nameList[i]);
}
+ free(nameList);
out:
- free(nameList);
return ret;
}
This changes the output from:
Attempting to connect to hypervisor
Connected to hypervisor at "qemu:///system"
Hypervisor: "QEMU" version: 0.32.656
There are 0 active and 2 inactive domains
Inactive domains:
foo
bar
Disconnected from hypervisor
to
Attempting to connect to hypervisor
Connected to hypervisor at "qemu:///system"
Hypervisor: "QEMU" version: 0.32.656
There are 0 active and 2 inactive domains
foo (non-active)
bar (non-active)
Disconnected from hypervisor
John