On 10.04.2013 12:08, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Currently the virCgroupPtr struct contains 3 pieces of
> information
>
> - path - path of the cgroup, relative to current process'
> cgroup placement
> - placement - current process' placement in each controller
> - mounts - mount point of each controller
>
> When reading/writing cgroup settings, the path & placement
> strings are combined to form the file path. This approach
> only works if we assume all cgroups will be relative to
> the current process' cgroup placement.
>
> To allow support for managing cgroups at any place in the
> heirarchy a change is needed. The 'placement' data should
> reflect the absolute path to the cgroup, and the 'path'
> value should no longer be used to form the paths to the
> cgroup attribute files.
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> src/util/vircgroup.c | 222 +++++++++++++++++++++++++++++++++++---------------
> tests/vircgrouptest.c | 53 +++++++-----
> 2 files changed, 188 insertions(+), 87 deletions(-)
>
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 2f52c92..c336806 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -101,6 +101,23 @@ bool virCgroupHasController(virCgroupPtr cgroup, int controller)
> }
>
> #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
> +static int virCgroupCopyMounts(virCgroupPtr group,
> + virCgroupPtr parent)
> +{
> + int i;
> + for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) {
> + if (!parent->controllers[i].mountPoint)
> + continue;
> +
> + group->controllers[i].mountPoint =
> + strdup(parent->controllers[i].mountPoint);
> +
> + if (!group->controllers[i].mountPoint)
> + return -ENOMEM;
> + }
> + return 0;
> +}
> +
> /*
> * Process /proc/mounts figuring out what controllers are
> * mounted and where
> @@ -158,12 +175,61 @@ no_memory:
> }
>
>
> +static int virCgroupCopyPlacement(virCgroupPtr group,
> + const char *path,
> + virCgroupPtr parent)
> +{
> + int i;
> + for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) {
> + if (!group->controllers[i].mountPoint)
> + continue;
> +
> + if (path[0] == '/') {
> + if (!(group->controllers[i].placement = strdup(path)))
> + return -ENOMEM;
> + } else {
> + /*
> + * parent=="/" + path="" => "/"
> + * parent=="/libvirt.service" + path="" => "/libvirt.service"
> + * parent=="/libvirt.service" + path="foo" => "/libvirt.service/foo"
> + */
s/path=/path==/
> + if (virAsprintf(&group->controllers[i].placement,
> + "%s%s%s",
> + parent->controllers[i].placement,
> + (STREQ(parent->controllers[i].placement, "/") ||
> + STREQ(path, "") ? "" : "/"),
> + path) < 0)
No, please no. This is too big for my small brain. And it's easy to make
a mistake here, as you just did. The closing parenthesis should be just
before the ternary operator. In fact, both parentheses can be left out.
> + return -ENOMEM;
> + }
> + }
> +
> + return 0;
> +}
> +
> +
> /*
Insert function name here.
> + * @group: the group to process
> + * @path: the relative path to append, not starting with '/'
> + *
> * Process /proc/self/cgroup figuring out what cgroup
> * sub-path the current process is assigned to. ie not
> - * necessarily in the root
> + * necessarily in the root. The contents of this file
> + * looks like
> + *
> + * 9:perf_event:/
> + * 8:blkio:/
> + * 7:net_cls:/
> + * 6:freezer:/
> + * 5:devices:/
> + * 4:memory:/
> + * 3:cpuacct,cpu:/
> + * 2:cpuset:/
> + * 1:name=systemd:/user/berrange/2
> + *
> + * It then appends @path to each detected path.
> */
> -static int virCgroupDetectPlacement(virCgroupPtr group)
> +static int virCgroupDetectPlacement(virCgroupPtr group,
> + const char *path)
> {
> int i;
> FILE *mapping = NULL;
> @@ -177,18 +243,18 @@ static int virCgroupDetectPlacement(virCgroupPtr group)
>
> while (fgets(line, sizeof(line), mapping) != NULL) {
> char *controllers = strchr(line, ':');
> - char *path = controllers ? strchr(controllers+1, ':') : NULL;
> - char *nl = path ? strchr(path, '\n') : NULL;
> + char *selfpath = controllers ? strchr(controllers + 1, ':') : NULL;
> + char *nl = selfpath ? strchr(selfpath, '\n') : NULL;
>
> - if (!controllers || !path)
> + if (!controllers || !selfpath)
> continue;
>
> if (nl)
> *nl = '\0';
>
> - *path = '\0';
> + *selfpath = '\0';
> controllers++;
> - path++;
> + selfpath++;
>
> for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) {
> const char *typestr = virCgroupControllerTypeToString(i);
> @@ -198,14 +264,25 @@ static int virCgroupDetectPlacement(virCgroupPtr group)
> char *next = strchr(tmp, ',');
> int len;
> if (next) {
> - len = next-tmp;
> + len = next - tmp;
> next++;
> } else {
> len = strlen(tmp);
> }
> - if (typelen == len && STREQLEN(typestr, tmp, len) &&
> - !(group->controllers[i].placement = strdup(STREQ(path, "/") ? "" : path)))
> - goto no_memory;
> +
> + /*
> + * selfpath=="/" + path="" -> "/"
> + * selfpath=="/libvirt.service" + path="" -> "/libvirt.service"
> + * selfpath=="/libvirt.service" + path="foo" -> "/libvirt.service/foo"
> + */
> + if (typelen == len && STREQLEN(typestr, tmp, len)) {
> + if (virAsprintf(&group->controllers[i].placement,
> + "%s%s%s", selfpath,
> + (STREQ(selfpath, "/") ||
> + STREQ(path, "") ? "" : "/"),
same applies here
> + path) < 0)
> + goto no_memory;
> + }
>
> tmp = next;
> }
> diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
> index a68aa88..3f35f2e 100644
> --- a/tests/vircgrouptest.c
> +++ b/tests/vircgrouptest.c
> @@ -246,4 +255,4 @@ mymain(void)
> return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE;
> }
>
> -VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/libvircgroupmock.so")
> +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/vircgroupmock.so")
>
This seems a bit unrelated. It's fixing pre-existing bug so it should go
into separate patch.
ACK to the changes if you address those nits I've pointed out. But
please split this patch into two.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list