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

19 Jul 2013


      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

    