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

On 07/18/2013 09:00 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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(a)redhat.com&gt; > --- > 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.

> @@ -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.