3:29 a.m.
Dave Allan <dallan(a)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