1:51 p.m.
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(a)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;
+}
5:06 a.m.
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
11:05 p.m.
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(a)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;
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
9:46 p.m.
Jim Meyering wrote:
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:
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
3:19 a.m.
Dave Allan <dallan(a)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.
9:40 p.m.
New subject: [libvirt] [PATCH 0/3] A small example program
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
7:10 a.m.
New subject: [libvirt] Re: [PATCH 0/3] A small example program
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(a)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(a)/src/libvirt.la
+hellolibvirt_LDADD = $(top_builddir)/src/libvirt.la
--
1.6.1.2.418.gd79e6
7:32 a.m.
New subject: [libvirt] Re: [PATCH 0/3] A small example program
Jim Meyering <jim(a)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.
7:40 a.m.
New subject: [libvirt] Re: [PATCH 0/3] A small example program
On Fri, Jan 30, 2009 at 02:32:00PM +0100, Jim Meyering wrote:
Jim Meyering <jim(a)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 :|
8:54 a.m.
New subject: [libvirt] Re: [PATCH 0/3] A small example program
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Fri, Jan 30, 2009 at 02:32:00PM +0100, Jim Meyering wrote:
> Jim Meyering <jim(a)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(a)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
9:08 a.m.
New subject: [libvirt] Re: [PATCH 0/3] A small example program
On Fri, Jan 30, 2009 at 03:54:17PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)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(a)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 :|
1:57 p.m.
New subject: [libvirt] Re: [PATCH 0/3] A small example program
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> From 0131996b3cd819624259d6adcc5d968d6a0210b1 Mon Sep 17 00:00:00
2001
> From: Jim Meyering <meyering(a)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.
7:42 a.m.
New subject: [libvirt] Re: [PATCH 0/3] A small example program
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 :|
10:23 a.m.
New subject: [libvirt] Re: [PATCH 0/3] A small example program
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
10:27 a.m.
New subject: [libvirt] Re: [PATCH 0/3] A small example program
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 :|
4:46 p.m.
New subject: [libvirt] [PATCH 0/1] Add error reporting to example
The following patch adds error reporting to the example code.
4:46 p.m.
New subject: [libvirt] [PATCH] Add showError function to display error data generated by libvirt.
---
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
2:31 p.m.
New subject: [libvirt] Re: [PATCH] Add showError function to display error data generated by libvirt.
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
9:40 p.m.
New subject: [libvirt] [PATCH 1/3] Add a trivial example program
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
9:40 p.m.
New subject: [libvirt] [PATCH 2/3] Changes per Jim Meyering
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
9:40 p.m.
New subject: [libvirt] [PATCH 3/3] Add autoconf/automake support
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(a)/src/libvirt.la
--
1.6.0.6
4:13 a.m.
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
5806
days inactive
5818
days old
22 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Dave Allan -
David Allan
-
Jim Meyering -
Richard W.M. Jones