Remove 'cleanup' label and simplify remembering of the returned value
from the callback.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 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;
         }
     }

     if (!backends || !backendAvailable) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("no cgroup backend available"));
-        ret = -1;
-        goto cleanup;
+        return -1;
     }

- cleanup:
     return ret;
 }

-- 
2.26.2

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@redhat.com>
> ---
>  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.

>          }
>      }
> 
>      if (!backends || !backendAvailable) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("no cgroup backend available"));
> -        ret = -1;
> -        goto cleanup;
> +        return -1;
>      }
> 
> - cleanup:
>      return ret;
>  }
> 
> -- 
> 2.26.2
> 

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@redhat.com>
> > ---
> >  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.

On Thu, Oct 22, 2020 at 11:55:06AM +0200, Peter Krempa wrote:
> 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@redhat.com>
> > > ---
> > >  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.

Good question, if the list of processes is the same in cgroups v1 and v2
it should be enough to call it only for one the cgroups, but I would
rather call it for both just to be on a safe side.

Using boolean sounds good.

Pavel