[libvirt] [PATCH 2/3] Convert the virCgroupKill* APIs to report errors

[Daniel P. Berrange]

On Thu, Jul 18, 2013 at 09:36:59PM -0600, Eric Blake wrote:
> On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > Instead of returning errno values, change the virCgroupKill*
> > APIs to fully report errors.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> > ---
> >  src/lxc/lxc_process.c |  15 +------
> >  src/util/vircgroup.c  | 108 ++++++++++++++++++++++++++++----------------------
> >  2 files changed, 62 insertions(+), 61 deletions(-)
> > 
> 
> > +++ b/src/lxc/lxc_process.c
> > @@ -769,19 +769,7 @@ int virLXCProcessStop(virLXCDriverPtr driver,
> >      }
> >  
> >      if (priv->cgroup) {
> > -        rc = virCgroupKillPainfully(priv->cgroup);
> > -        if (rc < 0) {
> > -            virReportSystemError(-rc, "%s",
> > -                                 _("Failed to kill container PIDs"));
> > -            rc = -1;
> > -            goto cleanup;
> > -        }
> > -        if (rc == 1) {
> > -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > -                           _("Some container PIDs refused to die"));
> 
> So we previously reported -errno or +1 for two different types of failure...
> 
> > -            rc = -1;
> > -            goto cleanup;
> > -        }
> > +        virCgroupKillPainfully(priv->cgroup);
> 
> ...but now don't report any failure at all.  Is this right?  Shouldn't
> this be:
> 
> if (virCgroupKillPainfully(priv->cgroup) < 0)
>     goto cleanup;
> 
> or even still distinguish the error message for processes that couldn't
> be killed (but maybe hoisting it into vircgroup.c)?

I'll add in

-        virCgroupKillPainfully(priv->cgroup);
+        rc = virCgroupKillPainfully(priv->cgroup);
+        if (rc < 0)
+            return -1;
+        if (rc > 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Some processes refused to die"));
+            return -1;
+        }

> > +        ret = virCgroupKillRecursive(group, signum);
> > +        VIR_DEBUG("Iteration %zu rc=%d", i, ret);
> > +        /* If ret == -1 we hit error, if 0 we ran out of PIDs */
> > +        if (ret <= 0)
> >              break;
> >  
> >          usleep(200 * 1000);
> >      }
> > -    VIR_DEBUG("Complete %d", rc);
> > -    return rc;
> > +    VIR_DEBUG("Complete %d", ret);
> > +    return ret;
> 
> Here, you can still return 1 without reporting any error, if you timed out.

Yep, I want to leave it upto the callers to decide if it is an error
condition.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|