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

[Eric Blake]

On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> Instead of returning errno values, change the virCgroupKill*
> APIs to fully report errors.
> 
> Signed-off-by: Daniel P. Berrange <berrange at 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)?

> +++ 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

s/kill/are killed/
s/non/none/

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

copy-and-paste

> +        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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130718/1ccc139e/attachment-0001.sig>