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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list