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@redhat.com>
Instead of returning errno values, change the virCgroupKill* APIs to fully report errors.
Signed-off-by: Daniel P. Berrange <berrange@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 :|