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