[libvirt] [PATCH] libvirt.c: fix virStateActive return value; document ...

[Daniel Veillard]

On Mon, Nov 24, 2008 at 04:31:28PM +0000, Daniel P. Berrange wrote:
> On Mon, Nov 24, 2008 at 03:54:30PM +0100, Jim Meyering wrote:
> > The s/1/-1/ fix induces no semantic change, since the sole use
> > of virStateActive tests solely for nonzero.
> > 
> > >From 4bc9713207a2ed6b101e2067f7bba82d1df45987 Mon Sep 17 00:00:00 2001
> > From: Jim Meyering <meyering at redhat.com>
> > Date: Mon, 24 Nov 2008 15:52:55 +0100
> > Subject: [PATCH] libvirt.c: fix virStateActive return value; document some new functions
> > 
> > * src/libvirt.c (virStateActive): Return -1 upon error, not 1,
> > to be consistent with the other virState* functions.
> > (virStateActive, virStateCleanup, virStateReload, virStateActive):
> > Add per-function comments.
> 
> NACK.
> 
> > +/**
> > + * virStateActive
> > + *
> > + * Run each virtualization driver's "active" method.
> > + *
> > + * Return 0 if successful, -1 upon error.
> > + */
> >  int virStateActive(void) {
> >      int i, ret = 0;
> > 
> >      for (i = 0 ; i < virStateDriverTabCount ; i++) {
> >          if (virStateDriverTab[i]->active &&
> >              virStateDriverTab[i]->active())
> > -            ret = 1;
> > +            ret = -1;
> 
> This is *not* an error condition. This method is basically asking
> whether the driver is 'active' - eg, does it have any domains
> running. It returns 0 if it isn't active, or 1 if it is active.
> 
> There is no error scenario - it can never fail.

  Then that should be explained in the (missing) comment!
Still +1 for other parts of that patch including the comments I assume,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/