On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)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);
@@ -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.
@@ -1019,22 +1023,28 @@ static int
virCgroupAddTaskStrController(virCgroupPtr group,
+ if (virCgroupAddTaskController(group, p, controller) < 0)
{
+ virErrorPtr err = virGetLastError();
+ /* A thread that exits between when we first read the source
+ * tasks and now is not fatal. */
+ if (err && err->code == VIR_ERR_SYSTEM_ERROR &&
+ err->int1 == ESRCH) {
shorter as 'if (virLastErrorIsSystemErrno(ESRCH))'
@@ -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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library
http://libvirt.org