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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]