9:33 a.m.
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(a)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(a)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 :|