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