Dave Allan <dallan@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