[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 9/8] hellolibvirt: Adjust some comments and condition usage



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]