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@xxxxxxxxxx>
> > ---
> > 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.
