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. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list