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

Re: [libvirt] [PATCH 1/8] hellolibvirt: Update hellolibvirt example



On Tue, Feb 19, 2013 at 11:26:34AM +0100, Michal Privoznik wrote:
> On 19.02.2013 03:14, John Ferlan wrote:
> > Add a list of active domains, list of active/inactive networks, and
> > list of active/inactive storage pools
> > ---
> >  examples/hellolibvirt/hellolibvirt.c | 201 ++++++++++++++++++++++++++++++-----
> >  1 file changed, 173 insertions(+), 28 deletions(-)
> > 
> > diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c
> > index 234637e..f191782 100644
> > --- a/examples/hellolibvirt/hellolibvirt.c
> > +++ b/examples/hellolibvirt/hellolibvirt.c
> > @@ -85,65 +85,200 @@ out:
> >      return ret;
> >  }
> >  
> > -
> > +typedef int (*virFunction)(virConnectPtr conn,
> > +                           char **nameList,
> > +                           int maxnames);
> >  static int
> > -showDomains(virConnectPtr conn)
> > +listObject(virConnectPtr conn, int maxnames, const char *objNameStr,
> > +           virFunction fcn)
> >  {
> > -    int ret = 0, i, numNames, numInactiveDomains, numActiveDomains;
> > +    int ret = 0, i, numNames;
> >      char **nameList = NULL;
> >  
> > -    numActiveDomains = virConnectNumOfDomains(conn);
> > -    if (-1 == numActiveDomains) {
> > +    nameList = malloc(sizeof(*nameList) * maxnames);
> > +
> > +    if (NULL == nameList) {
> 
> If blue is the sky .... These Yoda conditions should really be made
> inverted. But there are some even outside of the hellolibvirt.c.
> 
> >          ret = 1;
> > -        printf("Failed to get number of active domains\n");
> > +        printf("Could not allocate memory for list of %s\n", objNameStr);
> > +        goto out;
> > +    }
> > +
> > +    numNames = (*fcn)(conn, nameList, maxnames);
> > +
> > +    if (-1 == numNames) {
> > +        ret = 1;
> > +        printf("Could not get list of %s from hypervisor\n", objNameStr);
> >          showError(conn);
> >          goto out;
> >      }
> >  
> > -    numInactiveDomains = virConnectNumOfDefinedDomains(conn);
> > -    if (-1 == numInactiveDomains) {
> > +    printf("    %s: \n", objNameStr);
> > +    for (i = 0; i < numNames; i++) {
> > +        printf("\t%s\n", *(nameList + i));
> > +        /* The API documentation doesn't say so, but the names
> > +         * returned by are strdup'd and must be freed here.
> > +         */
> 
> The docs should be fixed then.
> 
> > +        free(*(nameList + i));
> > +    }
> > +
> > +out:
> > +    free(nameList);
> > +    return ret;
> > +}
> 
> I wonder if we should advise users to use the other list APIs that are
> around for a while (virConnectListAll. I guess it all boils down to
> question if the hellolibvirt binary is supposed to work with ancient
> libvirts or is just an example shipped within a release.
> In case it is supposed to work with prehistoric versions, we must add a
> fallback code. If we are satisfied with the example working with current
> libvirt, there's no need for fallbacking code then.

I originally wrote hellolibvirt because at the time (prehistoric
versions!) there was no good example of a trivial use of the library.
I think it's not a bad idea to keep it updated with modern code since
I'm guessing anybody looking at it probably intends to develop against
the version from which they got it.  I think it should illustrate the
most basic use of the API, so it should demonstrate best practices for
the functionality it implements, in this case, use the new API that
lists all domains rather than the two older calls and don't bother
with the fallback case.

Dave


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