Re: [libvirt] [PATCH v2 1/8] hellolibvirt: Update hellolibvirt example
On 02/22/2013 07:46 AM, John Ferlan wrote:
On 02/20/2013 08:05 PM, Dave Allan wrote:
> On Wed, Feb 20, 2013 at 12:38:38PM -0500, John Ferlan wrote:
>> Update the code to be more in line with how code looks elsewhere in
>> libvirt. Allow listing of domains, networks, storage pools, and
>> network interfaces.
> I like the changes to make the style more in line with the rest of the
> codebase, however, I really think that this example code should be
> about a minimal use of the API to list a few domains and that's all,
> so I'm not in favor of extending it to list other kinds of objects. I
> think people can find the details of how to do that kind of thing in
> the API docs.
>
> Dave
>
>> Update the function prototypes in libvirt.c to include a message about
>> the client needing to free() returned name fields. Fix the all domains
>> example flags values.
>> ---
>> examples/hellolibvirt/hellolibvirt.c | 131 ++++++++++++++++++++++-------------
>> src/libvirt.c | 21 +++---
>> 2 files changed, 91 insertions(+), 61 deletions(-)
Any other (strong) opinions one way or another? Should I remove the
hellolibvirt.c for now?
Adding network, volume groups, and interfaces was (mostly) trivial once
I got past the set up the structure to handle multiple types. It
certainly shows how similar the API's are for various objects and how
trivial it is to gather generic information for each.
That's a neat idea, but I think putting the actual functions being
called into a table and calling them indirectly obscures what's trying
to be exhibited.
Aside from that, the virInterface API *still* isn't available on all
platforms, so I don't think that's a good thing to have in a "basic"
example program - we would get too many noobs asking why the example
program is "broken".
I think it would be better to just provide simple examples for domain,
with a comment somewhere that says "storage pool, network, and interface
APIs work in a similar fashion" or something like that. (And maybe you
could have this more complicated example called hellolibvirt2.c or
something)
Any thoughts on the libvirt.c changes which are primarily documentation
of the need to free the 'names' returned. That was a result of a review
comment which noted that the comment in hellolibvirt.c if true should be
rectified. I forgot to put the v2->v1 differences marker when generating
the patch to call this one out specifically.
Those changes seem fine to me, although maybe they should be in a
separate patch, since they aren't really part of updating hellolibvirt.