On Thu, Apr 11, 2013 at 12:02:05PM +0200, Michal Privoznik wrote:
On 10.04.2013 12:08, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
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@redhat.com> --- 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.
It isn't a mistake - I just used () for style reasons - get the second line to indent, so that it was obviously a continuation. The () usage I had has no functional effect since || is higher precedence than ?:
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.
Yep, that's a mistake 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 :|