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

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(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 54eb728..b7d74a7 100644 --- a/src/lxc/lxc_process.c +++ 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")); - rc = -1; - goto cleanup; - } + virCgroupKillPainfully(priv->cgroup); } else { /* If cgroup doesn't exist, the VM pids must have already * died and so we're just cleaning up stale state @@ -792,7 +780,6 @@ int virLXCProcessStop(virLXCDriverPtr driver, rc = 0; -cleanup: return rc; } diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 12ace2e..fb76a13 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2318,9 +2318,12 @@ int virCgroupGetFreezerState(virCgroupPtr group, char **state) #if defined HAVE_KILL && defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R +/* + * Returns 1 if some PIDs kill, 0 if non are killed, or -1 on error + */ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr pids) { - int rc; + int ret = -1; bool killedAny = false; char *keypath = NULL; bool done = false; @@ -2328,10 +2331,11 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr VIR_DEBUG("group=%p path=%s signum=%d pids=%p", group, group->path, signum, pids); - rc = virCgroupPathOfController(group, -1, "tasks", &keypath); - if (rc != 0) { - VIR_DEBUG("No path of %s, tasks", group->path); - return rc; + if (virCgroupPathOfController(group, -1, "tasks", &keypath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No tasks file for group path '%s'"), + group->path); + return -1; } /* PIDs may be forking as we kill them, so loop @@ -2340,8 +2344,9 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr while (!done) { done = true; if (!(fp = fopen(keypath, "r"))) { - rc = -errno; - VIR_DEBUG("Failed to read %s: %m\n", keypath); + virReportSystemError(errno, + _("Failed to read %s"), + keypath); goto cleanup; } else { while (!feof(fp)) { @@ -2349,8 +2354,9 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr if (fscanf(fp, "%lu", &pid_value) != 1) { if (feof(fp)) break; - rc = -errno; - VIR_DEBUG("Failed to read %s: %m\n", keypath); + virReportSystemError(errno, + _("Failed to read %s"), + keypath); goto cleanup; } if (virHashLookup(pids, (void*)pid_value)) @@ -2360,7 +2366,9 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr /* Cgroups is a Linux concept, so this cast is safe. */ if (kill((pid_t)pid_value, signum) < 0) { if (errno != ESRCH) { - rc = -errno; + virReportSystemError(errno, + _("Failed to kill process %lu"), + pid_value); goto cleanup; } /* Leave RC == 0 since we didn't kill one */ @@ -2375,13 +2383,13 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr } } - rc = killedAny ? 1 : 0; + ret = killedAny ? 1 : 0; cleanup: VIR_FREE(keypath); VIR_FORCE_FCLOSE(fp); - return rc; + return ret; } @@ -2400,15 +2408,12 @@ static void *virCgroupPidCopy(const void *name) } /* - * Returns - * < 0 : errno that occurred - * 0 : no PIDs killed - * 1 : at least one PID killed + * Returns 1 if some PIDs kill, 0 if non are killed, or -1 on error */ int virCgroupKill(virCgroupPtr group, int signum) { VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); - int rc; + int ret; /* The 'tasks' file in cgroups can contain duplicated * pids, so we use a hash to track which we've already * killed. @@ -2420,37 +2425,42 @@ int virCgroupKill(virCgroupPtr group, int signum) virCgroupPidCopy, NULL); - rc = virCgroupKillInternal(group, signum, pids); + ret = virCgroupKillInternal(group, signum, pids); virHashFree(pids); - return rc; + return ret; } static int virCgroupKillRecursiveInternal(virCgroupPtr group, int signum, virHashTablePtr pids, bool dormdir) { + int ret = -1; int rc; - int killedAny = 0; + bool killedAny = false; char *keypath = NULL; DIR *dp; virCgroupPtr subgroup = NULL; struct dirent *ent; VIR_DEBUG("group=%p path=%s signum=%d pids=%p", group, group->path, signum, pids); - rc = virCgroupPathOfController(group, -1, "", &keypath); - if (rc != 0) { - VIR_DEBUG("No path of %s, tasks", group->path); - return rc; + if (virCgroupPathOfController(group, -1, "", &keypath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No tasks file for group path '%s'"), + group->path); + return -1; } - if ((rc = virCgroupKillInternal(group, signum, pids)) != 0) - return rc; + if ((rc = virCgroupKillInternal(group, signum, pids)) < 0) + return -1; + if (rc == 1) + killedAny = true; VIR_DEBUG("Iterate over children of %s", keypath); if (!(dp = opendir(keypath))) { - rc = -errno; - return rc; + virReportSystemError(errno, + _("Cannot open %s"), keypath); + return -1; } while ((ent = readdir(dp))) { @@ -2463,15 +2473,13 @@ static int virCgroupKillRecursiveInternal(virCgroupPtr group, int signum, virHas VIR_DEBUG("Process subdir %s", ent->d_name); - if (virCgroupNew(ent->d_name, group, -1, &subgroup) < 0) { - rc = -EIO; + if (virCgroupNew(ent->d_name, group, -1, &subgroup) < 0) goto cleanup; - } if ((rc = virCgroupKillRecursiveInternal(subgroup, signum, pids, true)) < 0) goto cleanup; if (rc == 1) - killedAny = 1; + killedAny = true; if (dormdir) virCgroupRemove(subgroup); @@ -2479,18 +2487,18 @@ static int virCgroupKillRecursiveInternal(virCgroupPtr group, int signum, virHas virCgroupFree(&subgroup); } - rc = killedAny; + ret = killedAny ? 1 : 0; cleanup: virCgroupFree(&subgroup); closedir(dp); - return rc; + return ret; } int virCgroupKillRecursive(virCgroupPtr group, int signum) { - int rc; + int ret; VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); virHashTablePtr pids = virHashCreateFull(100, NULL, @@ -2499,18 +2507,18 @@ int virCgroupKillRecursive(virCgroupPtr group, int signum) virCgroupPidCopy, NULL); - rc = virCgroupKillRecursiveInternal(group, signum, pids, false); + ret = virCgroupKillRecursiveInternal(group, signum, pids, false); virHashFree(pids); - return rc; + return ret; } int virCgroupKillPainfully(virCgroupPtr group) { size_t i; - int rc; + int ret; VIR_DEBUG("cgroup=%p path=%s", group, group->path); for (i = 0; i < 15; i++) { int signum; @@ -2521,33 +2529,39 @@ int virCgroupKillPainfully(virCgroupPtr group) else signum = 0; /* Just check for existence */ - rc = virCgroupKillRecursive(group, signum); - VIR_DEBUG("Iteration %zu rc=%d", i, rc); - /* If rc == -1 we hit error, if 0 we ran out of PIDs */ - if (rc <= 0) + 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; } #else /* !(HAVE_KILL, HAVE_MNTENT_H, HAVE_GETMNTENT_R) */ int virCgroupKill(virCgroupPtr group ATTRIBUTE_UNUSED, int signum ATTRIBUTE_UNUSED) { - return -ENOSYS; + virReportSystemError(ENOSYS, "%s", + _("Control groups not supported on this platform")); + return -1; } int virCgroupKillRecursive(virCgroupPtr group ATTRIBUTE_UNUSED, int signum ATTRIBUTE_UNUSED) { - return -ENOSYS; + virReportSystemError(ENOSYS, "%s", + _("Control groups not supported on this platform")); + return -1; } int virCgroupKillPainfully(virCgroupPtr group ATTRIBUTE_UNUSED) { - return -ENOSYS; + virReportSystemError(ENOSYS, "%s", + _("Control groups not supported on this platform")); + return -1; } #endif /* HAVE_KILL, HAVE_MNTENT_H, HAVE_GETMNTENT_R */ -- 1.8.1.4