[libvirt] [PATCH] trivial libvirt example code

[Jim Meyering]

Dave Allan <dallan at redhat.com> wrote:
> Richard W.M. Jones wrote:
>> On Fri, Jan 23, 2009 at 02:51:02PM -0500, Dave Allan wrote:
>>> The examples directory doesn't have a trivial example of how to
>>> connect  to a hypervisor, make a few calls, and disconnect, so I
>>> put one  together.  I would appreciate any suggestions on anything
>>> that I've done  wrong as well as suggestions for other fundamental
>>> API calls that should  be illustrated.
>>
>> Yes, I checked this example code and it is fine.  My only comment
>> would be on:
>>
>>> +    /* virConnectOpenAuth called here with all default parameters */
>>> +    conn = virConnectOpenAuth(NULL, virConnectAuthPtrDefault, 0);
>>
>> It might be better to let people connect to a named URI.
>>
>> Another possibility is to default to the test URI (test:///default)
>> since that (almost) always exists.
>
> Hi Rich,
>
> Thanks for taking a look at it.  I added a little code to let the user
> specify a URI on the command line.  Do you think it is worth
> committing?

Hi Dave,

I like your example.
Thanks for preparing it.

Here are some suggestions:

> diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c
> new file mode 100644
> index 0000000..22d3309
> --- /dev/null
> +++ b/examples/hellolibvirt/hellolibvirt.c
> @@ -0,0 +1,151 @@
> +/* This file contains trivial example code to connect to the running
> + * hypervisor and gather a few bits of information.  */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <libvirt/libvirt.h>
> +
> +static int
> +showHypervisorInfo(virConnectPtr conn)
> +{
> +    int ret = 0;
> +    unsigned long hvVer, major, minor, release;
> +    const char *hvType;
> +
> +    /* virConnectGetType returns a pointer to a static string, so no
> +     * allocation or freeing is necessary; it is possible for the call
> +     * to fail if, for example, there is no connection to a
> +     * hypervisor, so check what it returns. */
> +    hvType = virConnectGetType(conn);
> +    if (NULL == hvType) {
> +        ret = 1;
> +        printf("Failed to get hypervisor type\n");
> +        goto out;
> +    }
> +
> +    if (0 != virConnectGetVersion(conn, &hvVer)) {
> +        ret = 1;
> +        printf("Failed to get hypervisor version\n");
> +        goto out;
> +    }
> +
> +    major = hvVer / 1000000;
> +    hvVer %= 1000000;
> +    minor = hvVer / 1000;
> +    release = hvVer % 1000;
> +
> +    printf("Hypervisor: \"%s\" version: %lu.%lu.%lu\n",
> +           hvType,
> +           major,
> +           minor,
> +           release);
> +

How about initializing ret = 1 above
and setting ret = 0 here to indicate success?
It's a close call, since that results in removal of
only two "ret = 1" assignments.

> +out:
> +    return ret;
> +}
> +
> +
> +static int
> +showDomains(virConnectPtr conn)
> +{
> +    int ret = 0, i, numNames, numInactiveDomains, numActiveDomains;
> +    char **nameList = NULL;
> +
> +    numActiveDomains = virConnectNumOfDomains(conn);
> +    numInactiveDomains = virConnectNumOfDefinedDomains(conn);

It'd be good to handle numInactiveDomains < 0 differently.
Currently it'll probably provoke a failed malloc, below.

> +    printf("There are %d active and %d inactive domains\n",
> +           numActiveDomains, numInactiveDomains);
> +
> +    nameList = malloc(sizeof(char *) * (unsigned int)numInactiveDomains);

Using the target variable name rather than the type is a
little more maintainable, in general, so set a good example:
And please drop the cast.  We hate casts, and besides, it's not needed.

       nameList = malloc(sizeof(*nameList) * numInactiveDomains);

> +    if (NULL == nameList) {
> +        ret = 1;
> +        printf("Could not allocate memory for list of inactive domains\n");
> +        goto out;
> +    }
> +
> +    numNames = virConnectListDefinedDomains(conn,
> +                                            nameList,
> +                                            numInactiveDomains);
> +
> +    if (-1 == numNames) {
> +        ret = 1;
> +        printf("Could not get list of defined domains from hypervisor\n");
> +        goto out;
> +    }
> +
> +    if (numNames > 0) {
> +        printf("Inactive domains:\n");
> +    }
> +
> +    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.  */
> +        free(*(nameList + i));
> +    }

Here's another case where you can save a line by initializing
ret=1 up front and setting ret=0 here.

> +out:
> +    if (NULL != nameList) {
> +        free(nameList);

The test for non-NULL-before-free is unnecessary,
since free is guaranteed to handle NULL properly.
So just call free:

       free(nameList);

In fact, if you run "make syntax-check" before making the
suggested change, it should detect and complain about this code.

> +    return ret;
> +}
> +
> +
> +int
> +main(int argc, char *argv[])
> +{
> +    int ret = 0;
> +    virConnectPtr conn = NULL;

The above initialization is unnecessary.

> +    char *uri = NULL;

This one can be adjusted, too:

> +    printf("Attempting to connect to hypervisor\n");
> +
> +    if (argc > 0) {
> +        uri = argv[1];
> +    }

I'd write it as follows,

       char *uri = (argc > 0 ? argv[1] : NULL);

so that it's clear the variable is defined unconditionally.
In libvirt, it's ok to use C99 declaration-after-stmt.

> +    /* virConnectOpenAuth is called here with all default parameters,
> +     * except, possibly, the URI of the hypervisor. */
> +    conn = virConnectOpenAuth(uri, virConnectAuthPtrDefault, 0);
> +
> +    if (NULL == conn) {
> +        ret = 1;
> +        printf("No connection to hypervisor\n");
> +        goto out;
> +    }
> +
> +    uri = virConnectGetURI(conn);
> +    if (NULL == uri) {
> +        ret = 1;
> +        printf("Failed to get URI for hypervisor connection\n");
> +        goto disconnect;
> +    }
> +
> +    printf("Connected to hypervisor at \"%s\"\n", uri);
> +    free(uri);
> +
> +    if (0 != showHypervisorInfo(conn)) {
> +        ret = 1;
> +        goto disconnect;
> +    }
> +
> +    if (0 != showDomains(conn)) {
> +        ret = 1;
> +        goto disconnect;
> +    }
> +
> +disconnect:
> +    if (0 != virConnectClose(conn)) {
> +        printf("Failed to disconnect from hypervisor\n");
> +    } else {
> +        printf("Disconnected from hypervisor\n");
> +    }

You can save 3 statements by hoisting the initialization of ret=1
and setting ret=0 here.

> +out:
> +    return ret;
> +}

I noticed that you're using the git mirror.  Good!  ;-)
When posting patches, please use "git format-patch".
That would have made it easier for me to apply and test
your patches.  As it is, I didn't do either because
"git am FILE" didn't work:

    $ git am dallan.patch
    Applying: trivial libvirt example code
    warning: examples/hellolibvirt/hellolibvirt.c has type 100755, expected 100644
    error: patch failed: examples/hellolibvirt/hellolibvirt.c:97
    error: examples/hellolibvirt/hellolibvirt.c: patch does not apply
    Patch failed at 0001 trivial libvirt example code
    When you have resolved this problem run "git am --resolved".
    If you would prefer to skip this patch, instead run "git am --skip".
    To restore the original branch and stop patching run "git am --abort".

Note the warning about permissions on hellolibvirt.c.
You can correct that by running "chmod a-x hellolibvirt.c".

Here are some contribution guidelines that generally make it
easier for maintainers/committers to deal with contributed patches,
(though some parts are specific to git-managed projects):

    http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob;f=HACKING;hb=HEAD