Re: [PATCH 04/15] virCgroupKillRecursive: Refactor cleanup

On Thu, Oct 22, 2020 at 11:50:01 +0200, Pavel Hrdina wrote: > On Thu, Oct 22, 2020 at 11:34:55AM +0200, Peter Krempa wrote: > > Remove 'cleanup' label and simplify remembering of the returned value > > from the callback. > > > > Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; > > --- > > src/util/vircgroup.c | 16 ++++------------ > > 1 file changed, 4 insertions(+), 12 deletions(-) > > > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > > index b74ec1a8fa..dafc6c98c0 100644 > > --- a/src/util/vircgroup.c > > +++ b/src/util/vircgroup.c > > @@ -2532,8 +2532,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, > > int > > virCgroupKillRecursive(virCgroupPtr group, int signum) > > { > > - int ret = 0; > > - int rc; > > + int ret = -1; > > size_t i; > > bool backendAvailable = false; > > virCgroupBackendPtr *backends = virCgroupBackendGetAll(); > > @@ -2544,24 +2543,17 @@ virCgroupKillRecursive(virCgroupPtr group, int signum) > > for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { > > if (backends && backends[i] && backends[i]->available()) { > > backendAvailable = true; > > - rc = backends[i]->killRecursive(group, signum, pids); > > - if (rc < 0) { > > - ret = -1; > > - goto cleanup; > > - } > > - if (rc > 0) > > - ret = rc; > > + if ((ret = backends[i]->killRecursive(group, signum, pids)) < 0) > > + return -1; > > This doesn't look correct. In case that both cgroups v1 and v2 are used > the first call could return 1 meaning that it managed to kill some > process but the second call would probably return 0 because the process > would be already gone. Does it in such case even make sense to call the second callback? If yes, then I'll probably rather change it such, that a boolean variable will be set to true if any of the callbacks returns 1 to make it more obvious.