[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]