[libvirt] [PATCH 3/3] Convert remainder of cgroups code to report errors

[Eric Blake]

On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> Convert the remainiing methods in vircgroup.c to report errors

s/remainiing/remaining/

> instead of returning errno values.
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  src/util/vircgroup.c | 559 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 314 insertions(+), 245 deletions(-)
> 

> @@ -645,29 +661,22 @@ static int virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group)
>      for (i = 0; i < ARRAY_CARDINALITY(inherit_values); i++) {
>          char *value;
>  
> -        rc = virCgroupGetValueStr(parent,
> -                                  VIR_CGROUP_CONTROLLER_CPUSET,
> -                                  inherit_values[i],
> -                                  &value);
> -        if (rc != 0) {
> -            virReportSystemError(-rc,
> -                                 _("Failed to get '%s'"), inherit_values[i]);
> +        if (virCgroupGetValueStr(parent,
> +                                 VIR_CGROUP_CONTROLLER_CPUSET,
> +                                 inherit_values[i],
> +                                 &value) < 0)

Aha - this cleans up something I noticed in 1/3.

> @@ -675,33 +684,23 @@ static int virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group)
>  

> -
> -    if (rc != 0) {
> -        virReportSystemError(-rc,
> -                             _("Failed to set '%s'"), filename);
> +    if (virCgroupSetValueU64(group,
> +                             VIR_CGROUP_CONTROLLER_MEMORY,
> +                             filename, 1) < 0)
>          return -1;
> -    }
>  
>      return 0;

Can code like this be simplified to:

return virCgroupSetValueU64(group,
                            VIR_CGROUP_CONTROLLER_MEMORY,
                            filename, 1);

> @@ -968,11 +964,11 @@ int virCgroupRemove(virCgroupPtr group)
>   * @group: The cgroup to add a task to
>   * @pid: The pid of the task to add
>   *
> - * Returns: 0 on success
> + * Returns: 0 on success, -1 on error
>   */
>  int virCgroupAddTask(virCgroupPtr group, pid_t pid)

Ouch - at least src/qemu/qemu_cgroup.c calls this function, and still
expects errno values:

        rc = virCgroupAddTask(cgroup_vcpu, priv->vcpupids[i]);
        if (rc < 0) {
            virReportSystemError(-rc,
                                 _("unable to add vcpu %zu task %d to
cgroup"),
                                 i, priv->vcpupids[i]);
            goto cleanup;
        }

I didn't look elsewhere; all I needed was one counterexample to state
your patch is incomplete until you audit all callers impacted by
semantic changes.

> @@ -1019,22 +1023,28 @@ static int virCgroupAddTaskStrController(virCgroupPtr group,

> +        if (virCgroupAddTaskController(group, p, controller) < 0) {
> +            virErrorPtr err = virGetLastError();
> +            /* A thread that exits between when we first read the source
> +             * tasks and now is not fatal.  */
> +            if (err && err->code == VIR_ERR_SYSTEM_ERROR &&
> +                err->int1 == ESRCH) {

shorter as 'if (virLastErrorIsSystemErrno(ESRCH))'

> @@ -1645,18 +1659,30 @@ int virCgroupSetBlkioDeviceWeight(virCgroupPtr group,

> +    if (stat(path, &sb) < 0) {
> +        virReportSystemError(errno,
> +                             _("Path '%s' is not accessible"),
> +                             path);
> +        return -1;
> +    }
>  
> -    if (!S_ISBLK(sb.st_mode))
> -        return -EINVAL;
> +    if (!S_ISBLK(sb.st_mode)) {
> +        virReportSystemError(errno,

errno is wrong here - stat() succeeded.  You want to explicitly report
EINVAL.

> @@ -2582,8 +2648,9 @@ static char *virCgroupIdentifyRoot(virCgroupPtr group)
>              return NULL;
>          }
>  
> -        ignore_value(VIR_STRNDUP_QUIET(ret, group->controllers[i].mountPoint,
> -                                       tmp - group->controllers[i].mountPoint));
> +        if (VIR_STRNDUP(ret, group->controllers[i].mountPoint,
> +                        tmp - group->controllers[i].mountPoint) < 0)
> +            return NULL;
>          return ret;

You can still use ignore_value() here rather than 'if', if desired,
since ret will be NULL if VIR_STRNDUP fails.

-- 
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/bc99ab70/attachment-0001.sig>