On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list