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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list