On Thu, Jul 18, 2013 at 09:59:47PM -0600, Eric Blake wrote:
On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
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@redhat.com> --- 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);
I'm not a fan of doing that, since it means it'll be re-arranged again if we ever put move code in this method.
@@ -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.
Fixed those.
@@ -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.
I don't like that kind of use of ignore_value(). It makes it really easy for someone else to screw things up later by adding in more code between the VIR_STRNDUP & the following 'return ret'. An explicit 'return NULL' is safe in that scenario. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|