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