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

[Eric Blake]

On 07/19/2013 11:31 AM, Daniel P. Berrange wrote:
>> Still incomplete - you fixed virCgroupAddTask callers, but didn't audit
>> for others.  At least in this file, we have:
> 
> Sigh, you're right. I clearly forgot to change any of the callers.
> 

> @@ -584,14 +584,12 @@ static int lxcDomainGetInfo(virDomainPtr dom,
>                             "%s", _("Cannot read cputime for domain"));
>              goto cleanup;
>          }
> -        if ((rc = virCgroupGetMemoryUsage(priv->cgroup, &(info->memory))) < 0) {
> -            virReportError(VIR_ERR_OPERATION_FAILED,
> -                           "%s", _("Cannot read memory usage for domain"));
> -            if (rc == -ENOENT) {
> -                /* Don't fail if we can't read memory usage due to a lack of
> -                 * kernel support */
> +        if (virCgroupGetMemoryUsage(priv->cgroup, &(info->memory)) < 0) {
> +            /* Don't fail if we can't read memory usage due to a lack of
> +             * kernel support */
> +            if (virLastErrorIsSystemErrno(ENOENT))
>                  info->memory = 0;

Doesn't this leave virError still set?  You need to free the last error.

> -            } else
> +            else
>                  goto cleanup;

which means you can fix this up to use {} on both sides of the if/else.

> @@ -1707,39 +1668,23 @@ static int lxcSetVcpuBWLive(virCgroupPtr cgroup, unsigned long long period,

> -cleanup:
> -    if (period) {
> -        rc = virCgroupSetCpuCfsPeriod(cgroup, old_period);
> -        if (rc < 0)
> -            virReportSystemError(-rc, "%s",
> -                                 _("Unable to rollback cpu bandwidth period"));
> -    }
> +error:
> +    if (period)
> +        virCgroupSetCpuCfsPeriod(cgroup, old_period);

If the rollback fails, do we want to preserve the original error message
instead of overwriting it with our cleanup path?

> +++ b/src/qemu/qemu_cgroup.c
> @@ -54,25 +54,23 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk,
>  {
>      virDomainObjPtr vm = opaque;
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> -    int rc;
> +    int ret;
>  
>      VIR_DEBUG("Process path %s for disk", path);
> -    rc = virCgroupAllowDevicePath(priv->cgroup, path,
> +    ret = virCgroupAllowDevicePath(priv->cgroup, path,
>                                    (disk->readonly ? VIR_CGROUP_DEVICE_READ
>                                     : VIR_CGROUP_DEVICE_RW));

Worth fixing the indentation?

> @@ -198,20 +188,14 @@ qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED,
>  {
>      virDomainObjPtr vm = opaque;
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> -    int rc;
> +    int ret;
>  
>      VIR_DEBUG("Process path '%s' for USB device", path);
> -    rc = virCgroupAllowDevicePath(priv->cgroup, path,
> +    ret = virCgroupAllowDevicePath(priv->cgroup, path,
>                                    VIR_CGROUP_DEVICE_RW);

Indentation.

> @@ -855,39 +763,22 @@ qemuSetupCgroupVcpuBW(virCgroupPtr cgroup,

> -cleanup:
> -    if (period) {
> -        rc = virCgroupSetCpuCfsPeriod(cgroup, old_period);
> -        if (rc < 0)
> -            virReportSystemError(-rc, "%s",
> -                                 _("Unable to rollback cpu bandwidth period"));
> -    }
> +error:
> +    if (period)
> +        ignore_value(virCgroupSetCpuCfsPeriod(cgroup, old_period));

Another instance of failed rollback possibly wiping out a useful error.

Almost there.  I think I'm comfortable acking this now, since I know
you'll fix it up, and in order to get this in before release freeze.

-- 
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/20130719/050459a7/attachment-0001.sig>