[libvirt] [PATCH] trivial libvirt example code
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. Dave commit 14531c0fa3680890086d61ae4d66aec525f9c9a0 Author: David Allan <dallan@redhat.com> Date: Fri Jan 23 14:27:39 2009 -0500 Added a trivial example program to illustrate connecting to the hypervisor and making some simple API calls. diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c new file mode 100644 index 0000000..aae79b8 --- /dev/null +++ b/examples/hellolibvirt/hellolibvirt.c @@ -0,0 +1,137 @@ +/* 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); + +out: + return ret; +} + + +static int +showDomains(virConnectPtr conn) +{ + int ret = 0, i, numNames, numInactiveDomains, numActiveDomains; + char **nameList = NULL; + + numActiveDomains = virConnectNumOfDomains(conn); + numInactiveDomains = virConnectNumOfDefinedDomains(conn); + + printf("There are %d active and %d inactive domains\n", + numActiveDomains, numInactiveDomains); + + nameList = malloc(sizeof(char *) * (unsigned int)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)); + } + +out: + if (NULL != nameList) { + free(nameList); + } + + return ret; +} + + +int +main(void) +{ + int ret = 0; + virConnectPtr conn = NULL; + + printf("Attempting to connect to hypervisor\n"); + + /* virConnectOpenAuth called here with all default parameters */ + conn = virConnectOpenAuth(NULL, virConnectAuthPtrDefault, 0); + + if (NULL == conn) { + ret = 1; + printf("No connection to hypervisor\n"); + goto out; + } + + printf("Connected to hypervisor\n"); + + 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"); + } + +out: + return ret; +}
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. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
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? Dave 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); + +out: + return ret; +} + + +static int +showDomains(virConnectPtr conn) +{ + int ret = 0, i, numNames, numInactiveDomains, numActiveDomains; + char **nameList = NULL; + + numActiveDomains = virConnectNumOfDomains(conn); + numInactiveDomains = virConnectNumOfDefinedDomains(conn); + + printf("There are %d active and %d inactive domains\n", + numActiveDomains, numInactiveDomains); + + nameList = malloc(sizeof(char *) * (unsigned int)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)); + } + +out: + if (NULL != nameList) { + free(nameList); + } + + return ret; +} + + +int +main(int argc, char *argv[]) +{ + int ret = 0; + virConnectPtr conn = NULL; + char *uri = NULL; + + printf("Attempting to connect to hypervisor\n"); + + if (argc > 0) { + uri = argv[1]; + } + + /* 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"); + } + +out: + return ret; +} commit c8f073fd4ff032b88dff78d0aae93576a8dcf035 Author: David Allan <dallan@redhat.com> Date: Mon Jan 26 23:37:21 2009 -0500 Added a little code to let the user specify the URI of the hypervisor on the command line, per the suggestion of Rich Jones. diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c index aae79b8..22d3309 100644 --- a/examples/hellolibvirt/hellolibvirt.c +++ b/examples/hellolibvirt/hellolibvirt.c @@ -97,15 +97,21 @@ out: int -main(void) +main(int argc, char *argv[]) { int ret = 0; virConnectPtr conn = NULL; + char *uri = NULL; printf("Attempting to connect to hypervisor\n"); - /* virConnectOpenAuth called here with all default parameters */ - conn = virConnectOpenAuth(NULL, virConnectAuthPtrDefault, 0); + if (argc > 0) { + uri = argv[1]; + } + + /* 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; @@ -113,7 +119,15 @@ main(void) goto out; } - printf("Connected to hypervisor\n"); + 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;
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
Jim Meyering wrote:
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:
Thanks for the style suggestions--that's one of the reasons I was sending the code around.
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.
In this case, I think that the error cases are very unlikely, so I made the initialization 0, but I agree, it could go either way. I left it as is for now.
+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.
Doh--thanks. I missed that those calls could fail.
+ 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);
Thanks on sizeof(char *) vs. sizeof(*nameList)--fixed. The cast was there because virConnectNumOfDefinedDomains returns a signed value to allow for returning -1 on error, but malloc expects an unsigned argument. gcc 4.3 -Wconversion complains about this situation [different behavior from gcc < 4.3] I've turned off that warning and removed the cast.
+ 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.
Removed. (make syntax-check does not complain, btw)
+ return ret; +} + + +int +main(int argc, char *argv[]) +{ + int ret = 0; + virConnectPtr conn = NULL;
The above initialization is unnecessary.
Fixed.
+ 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.
I tend not to use the ternary operator much, because I've seen it abused to write really obfuscated code, but you're right, this is a place where it makes things cleaner. Done.
In libvirt, it's ok to use C99 declaration-after-stmt.
Good to know.
+ /* 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".
Will do.
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".
The permissions problem is strange--it's 644 in my development tree, and the patch I sent has: diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c new file mode 100644 What would cause git-am to think it was 755?
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
Good stuff. When I have a patch like this that people have commented on and I've modified it slightly in response, what's the best way to re-submit it? When Rich responded, I submitted both the entire patch with the changes as well as the changes separately. I'll resend a patch when I've gotten git to squash the history properly to produce something usable from git-format-patch. Dave
Dave Allan <dallan@redhat.com> wrote: ...
+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.
Doh--thanks. I missed that those calls could fail.
The warning sign for me was that they were declared to be of type "int". I asked myself "why?": for something that's supposed to count, why allow negative numbers.
+ printf("There are %d active and %d inactive domains\n", + numActiveDomains, numInactiveDomains); + + nameList = malloc(sizeof(char *) * (unsigned int)numInactiveDomains); ... + 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.
Removed. (make syntax-check does not complain, btw)
Ah. thanks for mentioning that. The script that detects those detects "if (expr != NULL) free(expr)", but didn't bother to match the less common case where NULL is first: "if (NULL != expr) free(expr)". I've made the upstream source of that script detect your syntax, too: http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=471cbb075f7 so none of those will slip into libvirt either, once we do the next gnulib->libvirt sync. ...
I noticed that you're using the git mirror. Good! ;-) When posting patches, please use "git format-patch".
Will do.
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".
The permissions problem is strange--it's 644 in my development tree, and the patch I sent has: diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c new file mode 100644
What would cause git-am to think it was 755?
I'll investigate if it happens again.
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
Good stuff.
When I have a patch like this that people have commented on and I've modified it slightly in response, what's the best way to re-submit it? When Rich responded, I submitted both the entire patch with the changes as well as the changes separately.
Personally I find 2nd-iteration reviews to work best when the incremental patch is posted with either 1) the preceding or 2) the squashed/final patch. Otherwise, I have to apply the preceding patch and the new patch on separate git branches, then diff those branches to see the incremental. That's not frustrating and inefficient.
The following patches provide a small example program that illustrates how to connect to libvirt, make a few API calls and disconnect. The first patch is my first cut at it with feedback from Rich Jones. The second patch implements feedback from Jim Meyering. The third patch is completely new and implements (I think) integration with the libvirt build tools, so I would appreciate careful examination of what I've done there. Dave
Hi David, I've just applied your 3 patches and built the example. That went fine. Then I tried to run it like this: $ LIBVIRT_DEBUG=1 libvirtd 2> log & pid=$! $ ./hellolibvirt Attempting to connect to hypervisor Connected to hypervisor at "qemu:///session" ? -> Failed to get hypervisor version Disconnected from hypervisor [Exit 1] There was plenty in the log: DEBUG: libvirt.c: virInitialize (register drivers) DEBUG: libvirt.c: virRegisterDriver (registering Test as driver 0) DEBUG: libvirt.c: virRegisterNetworkDriver (registering Test as network driver 0) DEBUG: libvirt.c: virRegisterStorageDriver (registering Test as storage driver 0) DEBUG: libvirt.c: virRegisterDriver (registering Xen as driver 1) DEBUG: libvirt.c: virRegisterDriver (registering OPENVZ as driver 2) DEBUG: libvirt.c: virRegisterDriver (registering remote as driver 3) DEBUG: libvirt.c: virRegisterNetworkDriver (registering remote as network driver 1) DEBUG: libvirt.c: virRegisterStorageDriver (registering remote as storage driver 1) DEBUG: libvirt.c: virRegisterDeviceMonitor (registering remote as device driver 0) DEBUG: libvirt.c: virRegisterNetworkDriver (registering Network as network driver 2) DEBUG: libvirt.c: virRegisterStorageDriver (registering storage as storage driver 2) DEBUG: libvirt.c: virRegisterDeviceMonitor (registering halDeviceMonitor as device driver 1) DEBUG: libvirt.c: virRegisterDriver (registering QEMU as driver 4) DEBUG: libvirt.c: virRegisterDriver (registering LXC as driver 5) DEBUG: libvirt.c: virRegisterDriver (registering UML as driver 6) DEBUG: libvirt.c: virConnectOpen (name=) DEBUG: libvirt.c: do_open (no name, allowing driver auto-select) DEBUG: libvirt.c: do_open (trying driver 0 (Test) ...) DEBUG: libvirt.c: do_open (driver 0 Test returned DECLINED) DEBUG: libvirt.c: do_open (trying driver 1 (Xen) ...) DEBUG: libvirt.c: do_open (driver 1 Xen returned DECLINED) DEBUG: libvirt.c: do_open (trying driver 2 (OPENVZ) ...) DEBUG: libvirt.c: do_open (driver 2 OPENVZ returned DECLINED) DEBUG: libvirt.c: do_open (trying driver 3 (remote) ...) DEBUG: libvirt.c: do_open (driver 3 remote returned DECLINED) DEBUG: libvirt.c: do_open (trying driver 4 (QEMU) ...) DEBUG: libvirt.c: do_open (driver 4 QEMU returned SUCCESS) DEBUG: libvirt.c: do_open (network driver 0 Test returned DECLINED) DEBUG: libvirt.c: do_open (network driver 1 remote returned DECLINED) DEBUG: libvirt.c: do_open (network driver 2 Network returned SUCCESS) DEBUG: libvirt.c: do_open (storage driver 0 Test returned DECLINED) DEBUG: libvirt.c: do_open (storage driver 1 remote returned DECLINED) DEBUG: libvirt.c: do_open (storage driver 2 storage returned SUCCESS) DEBUG: libvirt.c: do_open (node driver 0 remote returned DECLINED) DEBUG: libvirt.c: do_open (node driver 1 halDeviceMonitor returned SUCCESS) DEBUG: libvirt.c: virConnectGetURI (conn=0xcbab10) DEBUG: libvirt.c: virConnectGetType (conn=0xcbab10) DEBUG: libvirt.c: virConnectGetVersion (conn=0xcbab10, hvVer=0x7fffbc541108) DEBUG: libvirt.c: virConnectClose (conn=0xcbab10) DEBUG: datatypes.c: virUnrefConnect (unref connection 0xcbab10 1) DEBUG: datatypes.c: virReleaseConnect (release connection 0xcbab10) Shutting down on signal 15 Then I repeated with the just-built libvirtd, instead of the installed one, but then I get a new error: $ kill $pid $ LIBVIRT_DEBUG=1 ../../qemud/libvirtd 2> log & $ ./hellolibvirt Attempting to connect to hypervisor Connected to hypervisor at "qemu:///session" ? -> libvir: error : Unknown failure Failed to get hypervisor version Disconnected from hypervisor [Exit 1] This is on F10 x86_64. Does the above work for you? Also, please change examples/hellolibvirt/Makefile.am to use $(...) rather than the obsolete @...@ notation. I.e., just apply the following to the rest when you squash those commits into one. Yes, I know there are quite a few existing uses of @...@, and I even wrote a patch to remove them, but made the mistake of also fixing some minor quoting bugs along the way, and haven't gotten around to separating it into two change sets. Finally, please add examples/hellolibvirt/hellolibvirt to the top-level .cvsignore and rerun sync-vcs-ignore-files to regenerate .gitignore and .hgignore to match.
From 27f0007067280e4fa93b8c1d3c1c203314bde285 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 30 Jan 2009 10:02:51 +0100 Subject: [PATCH] use $(var), rather than obsolete @var@ notation
--- examples/hellolibvirt/Makefile.am | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/hellolibvirt/Makefile.am b/examples/hellolibvirt/Makefile.am index 6ef2cc8..489bf8b 100644 --- a/examples/hellolibvirt/Makefile.am +++ b/examples/hellolibvirt/Makefile.am @@ -1,5 +1,5 @@ -INCLUDES = -I@top_srcdir@/include +INCLUDES = -I$(top_srcdir)/include noinst_PROGRAMS = hellolibvirt hellolibvirt_CFLAGS = $(WARN_CFLAGS) hellolibvirt_SOURCES = hellolibvirt.c -hellolibvirt_LDADD = @top_builddir@/src/libvirt.la +hellolibvirt_LDADD = $(top_builddir)/src/libvirt.la -- 1.6.1.2.418.gd79e6
Jim Meyering <jim@meyering.net> wrote:
I've just applied your 3 patches and built the example. That went fine. Then I tried to run it like this: ... $ kill $pid $ LIBVIRT_DEBUG=1 ../../qemud/libvirtd 2> log & $ ./hellolibvirt Attempting to connect to hypervisor Connected to hypervisor at "qemu:///session" ? -> libvir: error : Unknown failure Failed to get hypervisor version Disconnected from hypervisor [Exit 1]
This is on F10 x86_64.
FYI, I poked around in the server to see what was going wrong. qemudGetVersion calls qemudExtractVersion, which calls virCapabilitiesDefaultGuestEmulator, which compares the single guest cap and gets an arch mismatch: Breakpoint 1, virCapabilitiesDefaultGuestEmulator (caps=0x71fee0, ostype=0x4bbc40 "hvm", arch=0x4bb980 "i686", domain=0x4bbc44 "qemu") at capabilities.c:497 ... (gdb) p *caps $12 = { host = { arch = 0x71be60 "x86_64", nfeatures = 0, features = 0x0, offlineMigrate = 0, liveMigrate = 0, nmigrateTrans = 0, migrateTrans = 0x0, nnumaCell = 1, numaCell = 0x717430 }, So this test fails: "x86_64" != "i686" STREQ(caps->guests[i]->arch.name, arch) so qemudGetVersion does, too.
On Fri, Jan 30, 2009 at 02:32:00PM +0100, Jim Meyering wrote:
Jim Meyering <jim@meyering.net> wrote:
I've just applied your 3 patches and built the example. That went fine. Then I tried to run it like this: ... $ kill $pid $ LIBVIRT_DEBUG=1 ../../qemud/libvirtd 2> log & $ ./hellolibvirt Attempting to connect to hypervisor Connected to hypervisor at "qemu:///session" ? -> libvir: error : Unknown failure Failed to get hypervisor version Disconnected from hypervisor [Exit 1]
This is on F10 x86_64.
FYI, I poked around in the server to see what was going wrong. qemudGetVersion calls qemudExtractVersion, which calls virCapabilitiesDefaultGuestEmulator, which compares the single guest cap and gets an arch mismatch:
Hmm, it should use the native arch - virExtractVersionInfo needs fixing to call uname, and extract the native arch. Hardcoding i386 was sufficient, when we mandated that 'qemu' was always present, but now we allow either qemu or KVM, so we need to always use native. NB, i3/4/586 should be mapped to i686. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Jan 30, 2009 at 02:32:00PM +0100, Jim Meyering wrote:
Jim Meyering <jim@meyering.net> wrote:
I've just applied your 3 patches and built the example. That went fine. Then I tried to run it like this: ... $ kill $pid $ LIBVIRT_DEBUG=1 ../../qemud/libvirtd 2> log & $ ./hellolibvirt Attempting to connect to hypervisor Connected to hypervisor at "qemu:///session" ? -> libvir: error : Unknown failure Failed to get hypervisor version Disconnected from hypervisor [Exit 1]
This is on F10 x86_64.
FYI, I poked around in the server to see what was going wrong. qemudGetVersion calls qemudExtractVersion, which calls virCapabilitiesDefaultGuestEmulator, which compares the single guest cap and gets an arch mismatch:
Hmm, it should use the native arch - virExtractVersionInfo needs fixing to call uname, and extract the native arch. Hardcoding i386 was sufficient, when we mandated that 'qemu' was always present, but now we allow either qemu or KVM, so we need to always use native. NB, i3/4/586 should be mapped to i686.
I see you already had to do that once.
From 0131996b3cd819624259d6adcc5d968d6a0210b1 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 30 Jan 2009 15:41:39 +0100 Subject: [PATCH] fix qemud version reporting when qemu is not installed
* src/qemu_conf.c (uname_normalize): New function, factored out of... (qemudBuildCommandLine): ...here. Use the new function. (qemudExtractVersion): Use it here, rather than hard-coding "i686". --- src/qemu_conf.c | 27 ++++++++++++++++++--------- 1 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index b4ec733..972ea50 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -486,17 +486,33 @@ rewait: return ret; } +static void +uname_normalize (struct utsname *ut) +{ + uname(ut); + + /* Map i386, i486, i586 to i686. */ + if (ut->machine[0] == 'i' && + ut->machine[1] != '\0' && + ut->machine[2] == '8' && + ut->machine[3] == '6' && + ut->machine[4] == '\0') + ut->machine[1] = '6'; +} + int qemudExtractVersion(virConnectPtr conn, struct qemud_driver *driver) { const char *binary; struct stat sb; + struct utsname ut; if (driver->qemuVersion > 0) return 0; + uname_normalize(&ut); if ((binary = virCapabilitiesDefaultGuestEmulator(driver->caps, "hvm", - "i686", + ut.machine, "qemu")) == NULL) return -1; @@ -718,14 +734,7 @@ int qemudBuildCommandLine(virConnectPtr conn, char domid[50]; char *pidfile; - uname(&ut); - - /* Nasty hack make i?86 look like i686 to simplify next comparison */ - if (ut.machine[0] == 'i' && - ut.machine[2] == '8' && - ut.machine[3] == '6' && - !ut.machine[4]) - ut.machine[1] = '6'; + uname_normalize(&ut); virUUIDFormat(vm->def->uuid, uuid); -- 1.6.1.2.418.gd79e6
On Fri, Jan 30, 2009 at 03:54:17PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
Hmm, it should use the native arch - virExtractVersionInfo needs fixing to call uname, and extract the native arch. Hardcoding i386 was sufficient, when we mandated that 'qemu' was always present, but now we allow either qemu or KVM, so we need to always use native. NB, i3/4/586 should be mapped to i686.
I see you already had to do that once.
Oh yes, forgot about that.
From 0131996b3cd819624259d6adcc5d968d6a0210b1 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 30 Jan 2009 15:41:39 +0100 Subject: [PATCH] fix qemud version reporting when qemu is not installed
* src/qemu_conf.c (uname_normalize): New function, factored out of... (qemudBuildCommandLine): ...here. Use the new function. (qemudExtractVersion): Use it here, rather than hard-coding "i686".
ACK to this. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
From 0131996b3cd819624259d6adcc5d968d6a0210b1 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 30 Jan 2009 15:41:39 +0100 Subject: [PATCH] fix qemud version reporting when qemu is not installed
* src/qemu_conf.c (uname_normalize): New function, factored out of... (qemudBuildCommandLine): ...here. Use the new function. (qemudExtractVersion): Use it here, rather than hard-coding "i686".
ACK to this.
Committed.
On Fri, Jan 30, 2009 at 02:10:58PM +0100, Jim Meyering wrote:
Hi David,
I've just applied your 3 patches and built the example. That went fine. Then I tried to run it like this:
$ LIBVIRT_DEBUG=1 libvirtd 2> log & pid=$! $ ./hellolibvirt Attempting to connect to hypervisor Connected to hypervisor at "qemu:///session" ? -> Failed to get hypervisor version
This shows a place where the example program could use the libvirt error information - it should have received error message to the effect of 'Cannot find QEMU binary /usr/bin/qemu' when qemudExtractVersion failed to find the binary it wanted Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Fri, Jan 30, 2009 at 02:10:58PM +0100, Jim Meyering wrote:
Hi David,
I've just applied your 3 patches and built the example. That went fine. Then I tried to run it like this:
$ LIBVIRT_DEBUG=1 libvirtd 2> log & pid=$! $ ./hellolibvirt Attempting to connect to hypervisor Connected to hypervisor at "qemu:///session" ? -> Failed to get hypervisor version
This shows a place where the example program could use the libvirt error information - it should have received error message to the effect of
'Cannot find QEMU binary /usr/bin/qemu'
when qemudExtractVersion failed to find the binary it wanted
I should illustrate getting the error messages in the example. I'll put it in. Did you deliberately break your system to provoke the failure? I had to rename /usr/bin/qemu to get it. Dave
On Fri, Jan 30, 2009 at 11:23:29AM -0500, Dave Allan wrote:
Daniel P. Berrange wrote:
On Fri, Jan 30, 2009 at 02:10:58PM +0100, Jim Meyering wrote:
Hi David,
I've just applied your 3 patches and built the example. That went fine. Then I tried to run it like this:
$ LIBVIRT_DEBUG=1 libvirtd 2> log & pid=$! $ ./hellolibvirt Attempting to connect to hypervisor Connected to hypervisor at "qemu:///session" ? -> Failed to get hypervisor version
This shows a place where the example program could use the libvirt error information - it should have received error message to the effect of
'Cannot find QEMU binary /usr/bin/qemu'
when qemudExtractVersion failed to find the binary it wanted
I should illustrate getting the error messages in the example. I'll put it in. Did you deliberately break your system to provoke the failure? I had to rename /usr/bin/qemu to get it.
Just don't install QEMU package - just have KVM Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
The following patch adds error reporting to the example code.
--- examples/hellolibvirt/hellolibvirt.c | 46 ++++++++++++++++++++++++++++++++++ 1 files changed, 46 insertions(+), 0 deletions(-) diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c index cc1af0f..234637e 100644 --- a/examples/hellolibvirt/hellolibvirt.c +++ b/examples/hellolibvirt/hellolibvirt.c @@ -6,6 +6,43 @@ #include <stdio.h> #include <stdlib.h> #include <libvirt/libvirt.h> +#include <libvirt/virterror.h> + +static void +showError(virConnectPtr conn) +{ + int ret; + virErrorPtr err; + + err = malloc(sizeof(*err)); + if (NULL == err) { + printf("Could not allocate memory for error data\n"); + goto out; + } + + ret = virConnCopyLastError(conn, err); + + switch (ret) { + case 0: + printf("No error found\n"); + break; + + case -1: + printf("Parameter error when attempting to get last error\n"); + break; + + default: + printf("libvirt reported: \"%s\"\n", err->message); + break; + } + + virResetError(err); + free(err); + +out: + return; +} + static int showHypervisorInfo(virConnectPtr conn) @@ -22,12 +59,14 @@ showHypervisorInfo(virConnectPtr conn) if (NULL == hvType) { ret = 1; printf("Failed to get hypervisor type\n"); + showError(conn); goto out; } if (0 != virConnectGetVersion(conn, &hvVer)) { ret = 1; printf("Failed to get hypervisor version\n"); + showError(conn); goto out; } @@ -57,6 +96,7 @@ showDomains(virConnectPtr conn) if (-1 == numActiveDomains) { ret = 1; printf("Failed to get number of active domains\n"); + showError(conn); goto out; } @@ -64,6 +104,7 @@ showDomains(virConnectPtr conn) if (-1 == numInactiveDomains) { ret = 1; printf("Failed to get number of inactive domains\n"); + showError(conn); goto out; } @@ -85,6 +126,7 @@ showDomains(virConnectPtr conn) if (-1 == numNames) { ret = 1; printf("Could not get list of defined domains from hypervisor\n"); + showError(conn); goto out; } @@ -124,6 +166,7 @@ main(int argc, char *argv[]) if (NULL == conn) { ret = 1; printf("No connection to hypervisor\n"); + showError(conn); goto out; } @@ -131,6 +174,7 @@ main(int argc, char *argv[]) if (NULL == uri) { ret = 1; printf("Failed to get URI for hypervisor connection\n"); + showError(conn); goto disconnect; } @@ -150,6 +194,8 @@ main(int argc, char *argv[]) disconnect: if (0 != virConnectClose(conn)) { printf("Failed to disconnect from hypervisor\n"); + showError(conn); + ret = 1; } else { printf("Disconnected from hypervisor\n"); } -- 1.6.0.6
David Allan wrote:
--- examples/hellolibvirt/hellolibvirt.c | 46 ++++++++++++++++++++++++++++++++++ 1 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c index cc1af0f..234637e 100644 --- a/examples/hellolibvirt/hellolibvirt.c +++ b/examples/hellolibvirt/hellolibvirt.c @@ -6,6 +6,43 @@ #include <stdio.h> #include <stdlib.h> #include <libvirt/libvirt.h> +#include <libvirt/virterror.h> + +static void +showError(virConnectPtr conn) +{ + int ret; + virErrorPtr err; + + err = malloc(sizeof(*err)); + if (NULL == err) { + printf("Could not allocate memory for error data\n"); + goto out; + } + + ret = virConnCopyLastError(conn, err); + + switch (ret) { + case 0: + printf("No error found\n"); + break; + + case -1: + printf("Parameter error when attempting to get last error\n"); + break; + + default: + printf("libvirt reported: \"%s\"\n", err->message); + break; + } + + virResetError(err); + free(err); + +out: + return; +} +
static int showHypervisorInfo(virConnectPtr conn) @@ -22,12 +59,14 @@ showHypervisorInfo(virConnectPtr conn) if (NULL == hvType) { ret = 1; printf("Failed to get hypervisor type\n"); + showError(conn); goto out; }
if (0 != virConnectGetVersion(conn, &hvVer)) { ret = 1; printf("Failed to get hypervisor version\n"); + showError(conn); goto out; }
@@ -57,6 +96,7 @@ showDomains(virConnectPtr conn) if (-1 == numActiveDomains) { ret = 1; printf("Failed to get number of active domains\n"); + showError(conn); goto out; }
@@ -64,6 +104,7 @@ showDomains(virConnectPtr conn) if (-1 == numInactiveDomains) { ret = 1; printf("Failed to get number of inactive domains\n"); + showError(conn); goto out; }
@@ -85,6 +126,7 @@ showDomains(virConnectPtr conn) if (-1 == numNames) { ret = 1; printf("Could not get list of defined domains from hypervisor\n"); + showError(conn); goto out; }
@@ -124,6 +166,7 @@ main(int argc, char *argv[]) if (NULL == conn) { ret = 1; printf("No connection to hypervisor\n"); + showError(conn); goto out; }
@@ -131,6 +174,7 @@ main(int argc, char *argv[]) if (NULL == uri) { ret = 1; printf("Failed to get URI for hypervisor connection\n"); + showError(conn); goto disconnect; }
@@ -150,6 +194,8 @@ main(int argc, char *argv[]) disconnect: if (0 != virConnectClose(conn)) { printf("Failed to disconnect from hypervisor\n"); + showError(conn); + ret = 1; } else { printf("Disconnected from hypervisor\n"); }
Before I forget about this patch, if it looks ok, could someone commit it? Thanks, Dave
This example code illustrates connecting to the hypervisor and making some simple API calls. Added a little code to let the user specify the URI of the hypervisor on the command line, per the suggestion of Rich Jones. --- examples/hellolibvirt/hellolibvirt.c | 151 ++++++++++++++++++++++++++++++++++ 1 files changed, 151 insertions(+), 0 deletions(-) create mode 100644 examples/hellolibvirt/hellolibvirt.c 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); + +out: + return ret; +} + + +static int +showDomains(virConnectPtr conn) +{ + int ret = 0, i, numNames, numInactiveDomains, numActiveDomains; + char **nameList = NULL; + + numActiveDomains = virConnectNumOfDomains(conn); + numInactiveDomains = virConnectNumOfDefinedDomains(conn); + + printf("There are %d active and %d inactive domains\n", + numActiveDomains, numInactiveDomains); + + nameList = malloc(sizeof(char *) * (unsigned int)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)); + } + +out: + if (NULL != nameList) { + free(nameList); + } + + return ret; +} + + +int +main(int argc, char *argv[]) +{ + int ret = 0; + virConnectPtr conn = NULL; + char *uri = NULL; + + printf("Attempting to connect to hypervisor\n"); + + if (argc > 0) { + uri = argv[1]; + } + + /* 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"); + } + +out: + return ret; +} -- 1.6.0.6
Handle failure of virConnectNumOfDomains and virConnectNumOfDefinedDomains Changed malloc to use size of variable, not type Removed unneeded NULL check before free Removed unneeded initialization of several variables Changed assignment to use the ternary operator --- examples/hellolibvirt/hellolibvirt.c | 28 ++++++++++++++++++---------- 1 files changed, 18 insertions(+), 10 deletions(-) diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c index 22d3309..cc1af0f 100644 --- a/examples/hellolibvirt/hellolibvirt.c +++ b/examples/hellolibvirt/hellolibvirt.c @@ -1,6 +1,8 @@ /* This file contains trivial example code to connect to the running * hypervisor and gather a few bits of information. */ +#include <config.h> + #include <stdio.h> #include <stdlib.h> #include <libvirt/libvirt.h> @@ -52,12 +54,23 @@ showDomains(virConnectPtr conn) char **nameList = NULL; numActiveDomains = virConnectNumOfDomains(conn); + if (-1 == numActiveDomains) { + ret = 1; + printf("Failed to get number of active domains\n"); + goto out; + } + numInactiveDomains = virConnectNumOfDefinedDomains(conn); + if (-1 == numInactiveDomains) { + ret = 1; + printf("Failed to get number of inactive domains\n"); + goto out; + } printf("There are %d active and %d inactive domains\n", numActiveDomains, numInactiveDomains); - nameList = malloc(sizeof(char *) * (unsigned int)numInactiveDomains); + nameList = malloc(sizeof(*nameList) * numInactiveDomains); if (NULL == nameList) { ret = 1; @@ -88,10 +101,7 @@ showDomains(virConnectPtr conn) } out: - if (NULL != nameList) { - free(nameList); - } - + free(nameList); return ret; } @@ -100,14 +110,12 @@ int main(int argc, char *argv[]) { int ret = 0; - virConnectPtr conn = NULL; - char *uri = NULL; + virConnectPtr conn; + char *uri; printf("Attempting to connect to hypervisor\n"); - if (argc > 0) { - uri = argv[1]; - } + uri = (argc > 0 ? argv[1] : NULL); /* virConnectOpenAuth is called here with all default parameters, * except, possibly, the URI of the hypervisor. */ -- 1.6.0.6
Added examples/hellolibvirt to the parent autoconf/automake files and created Makefile.am --- Makefile.am | 2 +- configure.in | 3 ++- examples/hellolibvirt/Makefile.am | 5 +++++ 3 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 examples/hellolibvirt/Makefile.am diff --git a/Makefile.am b/Makefile.am index d4e42f1..928a93c 100644 --- a/Makefile.am +++ b/Makefile.am @@ -4,7 +4,7 @@ LCOV = lcov GENHTML = genhtml SUBDIRS = gnulib/lib include src qemud proxy docs gnulib/tests \ - python tests po examples/domain-events/events-c + python tests po examples/domain-events/events-c examples/hellolibvirt ACLOCAL_AMFLAGS = -I m4 -I gnulib/m4 diff --git a/configure.in b/configure.in index 493ea28..3db0976 100644 --- a/configure.in +++ b/configure.in @@ -1285,7 +1285,8 @@ AC_OUTPUT(Makefile src/Makefile include/Makefile docs/Makefile \ tests/xmconfigdata/Makefile \ tests/xencapsdata/Makefile \ tests/confdata/Makefile \ - examples/domain-events/events-c/Makefile) + examples/domain-events/events-c/Makefile \ + examples/hellolibvirt/Makefile) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Configuration summary]) diff --git a/examples/hellolibvirt/Makefile.am b/examples/hellolibvirt/Makefile.am new file mode 100644 index 0000000..6ef2cc8 --- /dev/null +++ b/examples/hellolibvirt/Makefile.am @@ -0,0 +1,5 @@ +INCLUDES = -I@top_srcdir@/include +noinst_PROGRAMS = hellolibvirt +hellolibvirt_CFLAGS = $(WARN_CFLAGS) +hellolibvirt_SOURCES = hellolibvirt.c +hellolibvirt_LDADD = @top_builddir@/src/libvirt.la -- 1.6.0.6
On Tue, Jan 27, 2009 at 12:05:44AM -0500, Dave Allan wrote:
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?
Yes, +1 Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
participants (5)
-
Daniel P. Berrange -
Dave Allan -
David Allan
-
Jim Meyering -
Richard W.M. Jones