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

On 07/18/2013 09:00 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > Instead of returning errno values, change the virCgroupKill* > APIs to fully report errors. > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > --- > 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)?

> + 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.