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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130226/ddfa62b2/attachment-0001.sig>