On 06/05/2012 02:16 AM, tangchen wrote:
> Introduce a new API to move all tasks from a cgroup to another cgroup
Again, authorship is incorrect for the purposes of 'git am'.
> ---
> src/libvirt_private.syms | 1 +
> src/util/cgroup.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
> src/util/cgroup.h | 2 +
> 3 files changed, 58 insertions(+), 0 deletions(-)
>
> @@ -791,6 +791,61 @@ int virCgroupAddTask(virCgroupPtr group, pid_t pid)
> return rc;
> }
>
> +static int virCgrouAddTaskStr(virCgroupPtr group, const char *pidstr)
s/CgrouAdd/CgroupAdd/
> +int virCgroupMoveTask(virCgroupPtr src_group, virCgroupPtr dest_group)
> +{
> + int rc = 0;
> + int i;
> + char *content, *value, *next;
> +
> + for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) {
> + /* Skip over controllers not mounted */
> + if (!src_group->controllers[i].mountPoint ||
> + !dest_group->controllers[i].mountPoint)
> + continue;
Should we insist that src_group and dest_group have the same set of
mounted controllers? I'm worried that if we call this function, but the
set of mounted controllers differs between the two sets, then we end up
moving processes between some controllers and stranding them in the
source for the remaining controllers.
> +
> + rc = virCgroupGetValueStr(src_group, i, "tasks", &content);
> + if (rc != 0)
> + break;
Should we try to undo any changes if we fail partway through? This just
breaks the outer 'for' loop and returns 0, instead of using 'goto cleanup'.
> +
> + value = content;
> + while((next = strchr(value, '\n')) != NULL) {
Coding style: space after 'while'
> + *next = '\0';
> + if ((rc = virCgrouAddTaskStr(dest_group, value) < 0))
> + goto cleanup;
> + value = next + 1;
> + }
> + if (*value != '\0') {
> + if ((rc = virCgrouAddTaskStr(dest_group, value) < 0))
Does it make sense to parse all the string into integers, just to format
the integers back into strings? Or would it be simpler to just cat the
contents of the 'tasks' file from the source into the destination
without bothering to interpret the date in transit?
> + goto cleanup;
> + }
> +
> + VIR_FREE(content);
> + }
> +
> + return 0;
> +
> +cleanup:
> + virCgroupMoveTask(dest_group, src_group);
Is this cleanup always correct, or is it only correct if 'dest_group'
started life empty? Should we at least log a warning message if a move
was partially attempted and then reverted, particularly if the reversion
attempt failed?
--
Eric Blake eblake@xxxxxxxxxx +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