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

27 Feb 2013


      On 02/26/2013 07:20 AM, John Ferlan wrote:

I would have put '---' here, since...
...
v3->v2 difference: Reduced the amount of change to a few comments and
                    adjusting the (NULL == xxx) and (-1 == xxx) checks
Since these are just documentation changes - once ACK'd is it OK to push
now or should I wait for after the freeze?
Tks,
...this information isn't useful in the final git log, but makes sense
in reviewing.  This patch is safe for 1.0.3 (as you point out, it is a
doc improvement, and can't cause any bugs in libvirtd)
...
John
---
 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?
...
@@ -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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

    