Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_cgroup.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 210cac9866..10fdc7444d 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -988,7 +988,6 @@ static void qemuRestoreCgroupState(virDomainObjPtr vm) { g_autofree char *mem_mask = NULL; - int empty = -1; qemuDomainObjPrivatePtr priv = vm->privateData; size_t i = 0; g_autoptr(virBitmap) all_nodes = NULL; @@ -1003,8 +1002,8 @@ qemuRestoreCgroupState(virDomainObjPtr vm) if (!(mem_mask = virBitmapFormat(all_nodes))) goto error; - if ((empty = virCgroupHasEmptyTasks(priv->cgroup, - VIR_CGROUP_CONTROLLER_CPUSET)) <= 0) + if (virCgroupHasEmptyTasks(priv->cgroup, + VIR_CGROUP_CONTROLLER_CPUSET) <= 0) goto error; if (virCgroupSetCpusetMems(priv->cgroup, mem_mask) < 0) -- 2.26.2

All other helper processes are moved to cgroup with QEMU emulator thread as we keep the root VM cgroup without any processes. This assumption is validated in qemuRestoreCgroupState() which is called when libvirtd is restarted and reconnected to all running VMs. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_dbus.c | 5 +++-- src/qemu/qemu_dbus.h | 3 ++- src/qemu/qemu_extdevice.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c index a0567e55a3..ffcf83e5da 100644 --- a/src/qemu/qemu_dbus.c +++ b/src/qemu/qemu_dbus.c @@ -148,7 +148,8 @@ qemuDBusStop(virQEMUDriverPtr driver, int qemuDBusSetupCgroup(virQEMUDriverPtr driver, - virDomainObjPtr vm) + virDomainObjPtr vm, + virCgroupPtr cgroup) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; @@ -167,7 +168,7 @@ qemuDBusSetupCgroup(virQEMUDriverPtr driver, return -1; } - return virCgroupAddProcess(priv->cgroup, cpid); + return virCgroupAddProcess(cgroup, cpid); } int diff --git a/src/qemu/qemu_dbus.h b/src/qemu/qemu_dbus.h index 3c2145a223..e3ce1330fd 100644 --- a/src/qemu/qemu_dbus.h +++ b/src/qemu/qemu_dbus.h @@ -35,4 +35,5 @@ int qemuDBusVMStateAdd(virDomainObjPtr vm, const char *id); void qemuDBusVMStateRemove(virDomainObjPtr vm, const char *id); int qemuDBusSetupCgroup(virQEMUDriverPtr driver, - virDomainObjPtr vm); + virDomainObjPtr vm, + virCgroupPtr cgroup); diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index 2fb71dd334..8fe7ceaa10 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -280,7 +280,7 @@ qemuExtDevicesSetupCgroup(virQEMUDriverPtr driver, virDomainDefPtr def = vm->def; size_t i; - if (qemuDBusSetupCgroup(driver, vm) < 0) + if (qemuDBusSetupCgroup(driver, vm, cgroup) < 0) return -1; for (i = 0; i < def->nvideos; i++) { -- 2.26.2

When libvirtd starts a VM it internally stores a path to the main cgroup. When we restart libvirtd we should get to the same state. When we start a VM on host with systemd the cgroup is created for us and the process is already placed into that cgroup and we detect the path created by systemd using /proc/$PID/cgroup. After that we create sub-cgroups and move all threads there. Once libvirtd is restarted we again detect the cgroup path using /proc/$PID/cgroup, but in this case we will get a different path because the main thread was moved to a "emulator" cgroup. Instead of ignoring the "emulator" directory when validating cgroups remove it completely when detecting cgroup otherwise cgroups will not work properly when libvirtd is restarted. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroupv2.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 65f5cc6bd0..fb97b7fd2a 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -122,12 +122,6 @@ virCgroupV2ValidateMachineGroup(virCgroupPtr group, if (!(tmp = strrchr(group->unified.placement, '/'))) return false; - if (STREQ(tmp, "/emulator")) { - *tmp = '\0'; - - if (!(tmp = strrchr(group->unified.placement, '/'))) - return false; - } tmp++; if (STRNEQ(tmp, partmachinename) && @@ -198,6 +192,9 @@ virCgroupV2DetectPlacement(virCgroupPtr group, const char *controllers, const char *selfpath) { + g_autofree char *placement = g_strdup(selfpath); + char *tmp = NULL; + if (group->unified.placement) return 0; @@ -208,12 +205,18 @@ virCgroupV2DetectPlacement(virCgroupPtr group, if (STRNEQ(controllers, "")) return 0; + /* Running VM will have the main thread placed in emulator cgroup + * but we need to get the main cgroup. */ + tmp = g_strrstr(placement, "/emulator"); + if (tmp) + *tmp = '\0'; + /* * selfpath == "/" + path="" -> "/" * selfpath == "/libvirt.service" + path == "" -> "/libvirt.service" * selfpath == "/libvirt.service" + path == "foo" -> "/libvirt.service/foo" */ - group->unified.placement = g_strdup_printf("%s%s%s", selfpath, + group->unified.placement = g_strdup_printf("%s%s%s", placement, (STREQ(selfpath, "/") || STREQ(path, "") ? "" : "/"), path); return 0; -- 2.26.2

It is only used for debug and error purposes which can be easily replaced by @placement. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroup.c | 20 ++++++-------------- src/util/vircgrouppriv.h | 2 -- src/util/vircgroupv1.c | 8 +++----- src/util/vircgroupv2.c | 4 +--- tests/vircgrouptest.c | 29 ++++++++++------------------- 5 files changed, 20 insertions(+), 43 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index e3c45e93cb..7ff23e1218 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -669,13 +669,6 @@ virCgroupNew(pid_t pid, *group = NULL; newGroup = g_new0(virCgroup, 1); - if (path[0] == '/' || !parent) { - newGroup->path = g_strdup(path); - } else { - newGroup->path = g_strdup_printf("%s%s%s", parent->path, - STREQ(parent->path, "") ? "" : "/", path); - } - if (virCgroupDetect(newGroup, pid, controllers, path, parent) < 0) return -1; @@ -2388,8 +2381,8 @@ virCgroupKillInternal(virCgroupPtr group, g_autofree char *keypath = NULL; bool done = false; FILE *fp = NULL; - VIR_DEBUG("group=%p path=%s signum=%d pids=%p", - group, group->path, signum, pids); + VIR_DEBUG("group=%p signum=%d pids=%p", + group, signum, pids); if (virCgroupPathOfController(group, controller, taskFile, &keypath) < 0) return -1; @@ -2471,8 +2464,8 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, g_autoptr(DIR) dp = NULL; struct dirent *ent; int direrr; - VIR_DEBUG("group=%p path=%s signum=%d pids=%p", - group, group->path, signum, pids); + VIR_DEBUG("group=%p signum=%d pids=%p", + group, signum, pids); if (virCgroupPathOfController(group, controller, "", &keypath) < 0) return -1; @@ -2532,7 +2525,7 @@ virCgroupKillRecursive(virCgroupPtr group, int signum) virCgroupBackendPtr *backends = virCgroupBackendGetAll(); g_autoptr(GHashTable) pids = g_hash_table_new_full(g_int64_hash, g_int64_equal, g_free, NULL); - VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); + VIR_DEBUG("group=%p signum=%d", group, signum); for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { if (backends && backends[i] && backends[i]->available()) { @@ -2563,7 +2556,7 @@ virCgroupKillPainfully(virCgroupPtr group) { size_t i; int ret; - VIR_DEBUG("cgroup=%p path=%s", group, group->path); + VIR_DEBUG("cgroup=%p", group); for (i = 0; i < 15; i++) { int signum; if (i == 0) @@ -3491,7 +3484,6 @@ virCgroupFree(virCgroupPtr group) VIR_FREE(group->unified.mountPoint); VIR_FREE(group->unified.placement); - VIR_FREE(group->path); VIR_FREE(group); } diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index f85a36efdb..af8bf99888 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -60,8 +60,6 @@ typedef struct _virCgroupV2Controller virCgroupV2Controller; typedef virCgroupV2Controller *virCgroupV2ControllerPtr; struct _virCgroup { - char *path; - virCgroupBackendPtr backends[VIR_CGROUP_BACKEND_TYPE_LAST]; virCgroupV1Controller legacy[VIR_CGROUP_CONTROLLER_LAST]; diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 61fb359f8d..6bb819410b 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -545,7 +545,9 @@ virCgroupV1CpuSetInherit(virCgroupPtr parent, "cpuset.memory_migrate", }; - VIR_DEBUG("Setting up inheritance %s -> %s", parent->path, group->path); + VIR_DEBUG("Setting up inheritance %s -> %s", + parent->legacy[VIR_CGROUP_CONTROLLER_CPUSET].placement, + group->legacy[VIR_CGROUP_CONTROLLER_CPUSET].placement); for (i = 0; i < G_N_ELEMENTS(inherit_values); i++) { g_autofree char *value = NULL; @@ -583,7 +585,6 @@ virCgroupV1SetMemoryUseHierarchy(virCgroupPtr group) if (value == 1) return 0; - VIR_DEBUG("Setting up %s/%s", group->path, filename); if (virCgroupSetValueU64(group, VIR_CGROUP_CONTROLLER_MEMORY, filename, 1) < 0) @@ -601,7 +602,6 @@ virCgroupV1MakeGroup(virCgroupPtr parent, { size_t i; - VIR_DEBUG("Make group %s", group->path); for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { g_autofree char *path = NULL; @@ -671,7 +671,6 @@ virCgroupV1Remove(virCgroupPtr group) int rc = 0; size_t i; - VIR_DEBUG("Removing cgroup %s", group->path); for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { g_autofree char *grppath = NULL; @@ -697,7 +696,6 @@ virCgroupV1Remove(virCgroupPtr group) VIR_DEBUG("Removing cgroup %s and all child cgroups", grppath); rc = virCgroupRemoveRecursively(grppath); } - VIR_DEBUG("Done removing cgroup %s", group->path); return rc; } diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 530e5d2ce9..0467a72566 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -413,8 +413,6 @@ virCgroupV2MakeGroup(virCgroupPtr parent, return 0; } - VIR_DEBUG("Make group %s", group->path); - controller = virCgroupV2GetAnyController(group); if (virCgroupV2PathOfController(group, controller, "", &path) < 0) return -1; @@ -424,7 +422,7 @@ virCgroupV2MakeGroup(virCgroupPtr parent, if (!virFileExists(path) && (!create || (mkdir(path, 0755) < 0 && errno != EEXIST))) { virReportSystemError(errno, _("Failed to create v2 cgroup '%s'"), - group->path); + path); return -1; } diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index 6c1a766fa2..0dd5e3f0b4 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -38,7 +38,6 @@ VIR_LOG_INIT("tests.cgrouptest"); static int validateCgroup(virCgroupPtr cgroup, - const char *expectPath, const char **expectMountPoint, const char **expectLinkPoint, const char **expectPlacement, @@ -48,12 +47,6 @@ static int validateCgroup(virCgroupPtr cgroup, { size_t i; - if (STRNEQ(cgroup->path, expectPath)) { - fprintf(stderr, "Wrong path '%s', expected '%s'\n", - cgroup->path, expectPath); - return -1; - } - for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { if (STRNEQ_NULLABLE(expectMountPoint[i], cgroup->legacy[i].mountPoint)) { @@ -240,7 +233,7 @@ static int testCgroupNewForSelf(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "", mountsFull, links, placement, NULL, NULL, 0); + return validateCgroup(cgroup, mountsFull, links, placement, NULL, NULL, 0); } @@ -314,14 +307,14 @@ static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv); return -1; } - rv = validateCgroup(cgroup, "/virtualmachines.partition", mountsSmall, links, placementSmall, NULL, NULL, 0); + rv = validateCgroup(cgroup, mountsSmall, links, placementSmall, NULL, NULL, 0); virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/virtualmachines", true, -1, &cgroup)) != 0) { fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv); return -1; } - return validateCgroup(cgroup, "/virtualmachines.partition", mountsFull, links, placementFull, NULL, NULL, 0); + return validateCgroup(cgroup, mountsFull, links, placementFull, NULL, NULL, 0); } @@ -365,8 +358,7 @@ static int testCgroupNewForPartitionNested(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "/deployment.partition/production.partition", - mountsFull, links, placementFull, NULL, NULL, 0); + return validateCgroup(cgroup, mountsFull, links, placementFull, NULL, NULL, 0); } @@ -416,8 +408,7 @@ static int testCgroupNewForPartitionNestedDeep(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "/user/berrange.user/production.partition", - mountsFull, links, placementFull, NULL, NULL, 0); + return validateCgroup(cgroup, mountsFull, links, placementFull, NULL, NULL, 0); } @@ -448,7 +439,7 @@ static int testCgroupNewForPartitionDomain(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(domaincgroup, "/production.partition/foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); + return validateCgroup(domaincgroup, mountsFull, links, placement, NULL, NULL, 0); } static int testCgroupNewForPartitionDomainEscaped(const void *args G_GNUC_UNUSED) @@ -493,7 +484,7 @@ static int testCgroupNewForPartitionDomainEscaped(const void *args G_GNUC_UNUSED * since our fake /proc/cgroups pretends this controller * isn't compiled into the kernel */ - return validateCgroup(domaincgroup, "/_cgroup.evil/net_cls.evil/__evil.evil/_cpu.foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); + return validateCgroup(domaincgroup, mountsFull, links, placement, NULL, NULL, 0); } static int testCgroupNewForSelfAllInOne(const void *args G_GNUC_UNUSED) @@ -514,7 +505,7 @@ static int testCgroupNewForSelfAllInOne(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "", mountsAllInOne, linksAllInOne, placement, NULL, NULL, 0); + return validateCgroup(cgroup, mountsAllInOne, linksAllInOne, placement, NULL, NULL, 0); } @@ -547,7 +538,7 @@ static int testCgroupNewForSelfUnified(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "", empty, empty, empty, + return validateCgroup(cgroup, empty, empty, empty, "/not/really/sys/fs/cgroup", "/", controllers); } @@ -580,7 +571,7 @@ static int testCgroupNewForSelfHybrid(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "", mounts, empty, placement, + return validateCgroup(cgroup, mounts, empty, placement, "/not/really/sys/fs/cgroup/unified", "/", controllers); } -- 2.26.2

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroup.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 3db20ed262..a30aec4e5e 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -239,6 +239,23 @@ virCgroupSetBackends(virCgroupPtr group) } +static int +virCgroupCopyMounts(virCgroupPtr group, + virCgroupPtr parent) +{ + size_t i; + + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { + if (group->backends[i] && + group->backends[i]->copyMounts(group, parent) < 0) { + return -1; + } + } + + return 0; +} + + /* * Process /proc/mounts figuring out what controllers are * mounted and where @@ -375,12 +392,8 @@ virCgroupDetect(virCgroupPtr group, return -1; if (parent) { - for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { - if (group->backends[i] && - group->backends[i]->copyMounts(group, parent) < 0) { - return -1; - } - } + if (virCgroupCopyMounts(group, parent) < 0) + return -1; } else { if (virCgroupDetectMounts(group) < 0) return -1; -- 2.26.2

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroup.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index b5f38210fb..55fa49a398 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -393,6 +393,23 @@ virCgroupDetectPlacement(virCgroupPtr group, } +static int +virCgroupValidatePlacement(virCgroupPtr group, + pid_t pid) +{ + size_t i; + + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { + if (group->backends[i] && + group->backends[i]->validatePlacement(group, pid) < 0) { + return -1; + } + } + + return 0; +} + + static int virCgroupDetect(virCgroupPtr group, pid_t pid, @@ -430,12 +447,8 @@ virCgroupDetect(virCgroupPtr group, return -1; /* Check that for every mounted controller, we found our placement */ - for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { - if (group->backends[i] && - group->backends[i]->validatePlacement(group, pid) < 0) { - return -1; - } - } + if (virCgroupValidatePlacement(group, pid) < 0) + return -1; for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { if (group->backends[i]) { -- 2.26.2

The current code uses virCgroupNew() as a single point of entry and calls into virCgroupDetect() as well. Both have logic for several paths which is difficult to figure out. Extract the actually used code path from the two functions to make it obvious what's happening in this case. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroup.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index f21a581946..97e15c0ab3 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1048,7 +1048,28 @@ virCgroupNewDetect(pid_t pid, int controllers, virCgroupPtr *group) { - return virCgroupNew(pid, "", NULL, controllers, group); + g_autoptr(virCgroup) new = g_new0(virCgroup, 1); + + VIR_DEBUG("pid=%lld controllers=%d group=%p", + (long long) pid, controllers, group); + + if (virCgroupSetBackends(new) < 0) + return -1; + + if (virCgroupDetectMounts(new) < 0) + return -1; + + if (virCgroupDetectPlacement(new, pid, "") < 0) + return -1; + + if (virCgroupValidatePlacement(new, pid) < 0) + return -1; + + if (virCgroupDetectControllers(new, controllers, NULL) < 0) + return -1; + + *group = g_steal_pointer(&new); + return 0; } -- 2.26.2

Now it is always NULL. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroup.c | 40 +++++++++++++--------------------------- src/util/vircgrouppriv.h | 1 - src/util/vircgroupv1.c | 2 +- 3 files changed, 14 insertions(+), 29 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index b11139bef1..56aa9cc064 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -446,28 +446,19 @@ static int virCgroupDetect(virCgroupPtr group, pid_t pid, int controllers, - const char *path, - virCgroupPtr parent) + const char *path) { - VIR_DEBUG("group=%p controllers=%d path=%s parent=%p", - group, controllers, path, parent); + VIR_DEBUG("group=%p controllers=%d path=%s", + group, controllers, path); if (virCgroupSetBackends(group) < 0) return -1; - if (parent) { - if (virCgroupCopyMounts(group, parent) < 0) - return -1; - } else { - if (virCgroupDetectMounts(group) < 0) - return -1; - } + if (virCgroupDetectMounts(group) < 0) + return -1; - /* In some cases we can copy part of the placement info - * based on the parent cgroup... - */ - if (parent || path[0] == '/') { - if (virCgroupCopyPlacement(group, path, parent) < 0) + if (path[0] == '/') { + if (virCgroupCopyPlacement(group, path, NULL) < 0) return -1; } @@ -479,7 +470,7 @@ virCgroupDetect(virCgroupPtr group, if (virCgroupValidatePlacement(group, pid) < 0) return -1; - if (virCgroupDetectControllers(group, controllers, parent) < 0) + if (virCgroupDetectControllers(group, controllers, NULL) < 0) return -1; return 0; @@ -708,15 +699,12 @@ virCgroupMakeGroup(virCgroupPtr parent, /** * virCgroupNew: * @path: path for the new group - * @parent: parent group, or NULL * @controllers: bitmask of controllers to activate * * Create a new cgroup storing it in @group. * * If @path starts with a '/' it is treated as an - * absolute path, and @parent is ignored. Otherwise - * it is treated as being relative to @parent. If - * @parent is NULL, then the placement of the current + * absolute path. Otherwise then the placement of the current * process is used. * * Returns 0 on success, -1 on error @@ -724,19 +712,18 @@ virCgroupMakeGroup(virCgroupPtr parent, int virCgroupNew(pid_t pid, const char *path, - virCgroupPtr parent, int controllers, virCgroupPtr *group) { g_autoptr(virCgroup) newGroup = NULL; - VIR_DEBUG("pid=%lld path=%s parent=%p controllers=%d group=%p", - (long long) pid, path, parent, controllers, group); + VIR_DEBUG("pid=%lld path=%s controllers=%d group=%p", + (long long) pid, path, controllers, group); *group = NULL; newGroup = g_new0(virCgroup, 1); - if (virCgroupDetect(newGroup, pid, controllers, path, parent) < 0) + if (virCgroupDetect(newGroup, pid, controllers, path) < 0) return -1; *group = g_steal_pointer(&newGroup); @@ -940,7 +927,7 @@ virCgroupNewPartition(const char *path, tmp++; *tmp = '\0'; - if (virCgroupNew(-1, parentPath, NULL, controllers, &parent) < 0) + if (virCgroupNew(-1, parentPath, controllers, &parent) < 0) return -1; } @@ -1156,7 +1143,6 @@ virCgroupEnableMissingControllers(char *path, if (virCgroupNew(pidleader, "/", - NULL, controllers, &parent) < 0) return -1; diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index af8bf99888..0ac62882a5 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -112,7 +112,6 @@ int virCgroupGetValueForBlkDev(const char *str, int virCgroupNew(pid_t pid, const char *path, - virCgroupPtr parent, int controllers, virCgroupPtr *group); diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 6bb819410b..b3e0f733b9 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -1468,7 +1468,7 @@ virCgroupV1MemoryOnceInit(void) g_autoptr(virCgroup) group = NULL; unsigned long long int mem_unlimited = 0ULL; - if (virCgroupNew(-1, "/", NULL, -1, &group) < 0) + if (virCgroupNew(-1, "/", -1, &group) < 0) return; if (!virCgroupV1HasController(group, VIR_CGROUP_CONTROLLER_MEMORY)) -- 2.26.2

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroup.c | 52 +++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 794680d696..81aa3ee791 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -442,39 +442,6 @@ virCgroupDetectControllers(virCgroupPtr group, } -static int -virCgroupDetect(virCgroupPtr group, - pid_t pid, - int controllers, - const char *path) -{ - VIR_DEBUG("group=%p controllers=%d path=%s", - group, controllers, path); - - if (virCgroupSetBackends(group) < 0) - return -1; - - if (virCgroupDetectMounts(group) < 0) - return -1; - - if (virCgroupCopyPlacement(group, path, NULL) < 0) - return -1; - - /* ... but use /proc/cgroups to fill in the rest */ - if (virCgroupDetectPlacement(group, pid, path) < 0) - return -1; - - /* Check that for every mounted controller, we found our placement */ - if (virCgroupValidatePlacement(group, pid) < 0) - return -1; - - if (virCgroupDetectControllers(group, controllers, NULL) < 0) - return -1; - - return 0; -} - - char * virCgroupGetBlockDevString(const char *path) { @@ -717,7 +684,24 @@ virCgroupNew(pid_t pid, *group = NULL; newGroup = g_new0(virCgroup, 1); - if (virCgroupDetect(newGroup, pid, controllers, path) < 0) + if (virCgroupSetBackends(newGroup) < 0) + return -1; + + if (virCgroupDetectMounts(newGroup) < 0) + return -1; + + if (virCgroupCopyPlacement(newGroup, path, NULL) < 0) + return -1; + + /* ... but use /proc/cgroups to fill in the rest */ + if (virCgroupDetectPlacement(newGroup, pid, path) < 0) + return -1; + + /* Check that for every mounted controller, we found our placement */ + if (virCgroupValidatePlacement(newGroup, pid) < 0) + return -1; + + if (virCgroupDetectControllers(newGroup, controllers, NULL) < 0) return -1; *group = g_steal_pointer(&newGroup); -- 2.26.2

Now it is always -1. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroup.c | 16 +++++++--------- src/util/vircgrouppriv.h | 3 +-- src/util/vircgroupv1.c | 2 +- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 2c1d6f9f01..6a34eb4037 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -671,15 +671,14 @@ virCgroupMakeGroup(virCgroupPtr parent, * Returns 0 on success, -1 on error */ int -virCgroupNew(pid_t pid, - const char *path, +virCgroupNew(const char *path, int controllers, virCgroupPtr *group) { g_autoptr(virCgroup) newGroup = NULL; - VIR_DEBUG("pid=%lld path=%s controllers=%d group=%p", - (long long) pid, path, controllers, group); + VIR_DEBUG("path=%s controllers=%d group=%p", + path, controllers, group); *group = NULL; newGroup = g_new0(virCgroup, 1); @@ -694,11 +693,11 @@ virCgroupNew(pid_t pid, return -1; /* ... but use /proc/cgroups to fill in the rest */ - if (virCgroupDetectPlacement(newGroup, pid, path) < 0) + if (virCgroupDetectPlacement(newGroup, -1, path) < 0) return -1; /* Check that for every mounted controller, we found our placement */ - if (virCgroupValidatePlacement(newGroup, pid) < 0) + if (virCgroupValidatePlacement(newGroup, -1) < 0) return -1; if (virCgroupDetectControllers(newGroup, controllers, NULL) < 0) @@ -905,7 +904,7 @@ virCgroupNewPartition(const char *path, tmp++; *tmp = '\0'; - if (virCgroupNew(-1, parentPath, controllers, &parent) < 0) + if (virCgroupNew(parentPath, controllers, &parent) < 0) return -1; } @@ -1118,8 +1117,7 @@ virCgroupEnableMissingControllers(char *path, g_autoptr(virCgroup) parent = NULL; char *offset = path; - if (virCgroupNew(-1, - "/", + if (virCgroupNew("/", controllers, &parent) < 0) return -1; diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index 0ac62882a5..7248bc1d35 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -110,8 +110,7 @@ int virCgroupGetValueForBlkDev(const char *str, const char *devPath, char **value); -int virCgroupNew(pid_t pid, - const char *path, +int virCgroupNew(const char *path, int controllers, virCgroupPtr *group); diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index b3e0f733b9..98b5aa047d 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -1468,7 +1468,7 @@ virCgroupV1MemoryOnceInit(void) g_autoptr(virCgroup) group = NULL; unsigned long long int mem_unlimited = 0ULL; - if (virCgroupNew(-1, "/", -1, &group) < 0) + if (virCgroupNew("/", -1, &group) < 0) return; if (!virCgroupV1HasController(group, VIR_CGROUP_CONTROLLER_MEMORY)) -- 2.26.2

All callers pass true. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroup.c | 5 +---- src/util/vircgrouppriv.h | 1 - tests/vircgrouptest.c | 4 ++-- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 00967ea5fa..1e18b84b54 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -969,7 +969,6 @@ int virCgroupNewDomainPartition(virCgroupPtr partition, const char *driver, const char *name, - bool create, virCgroupPtr *group) { g_autofree char *grpname = NULL; @@ -993,7 +992,7 @@ virCgroupNewDomainPartition(virCgroupPtr partition, * a group for driver, is to avoid overhead to track * cumulative usage that we don't need. */ - if (virCgroupMakeGroup(partition, newGroup, create, + if (virCgroupMakeGroup(partition, newGroup, true, VIR_CGROUP_MEM_HIERACHY) < 0) { return -1; } @@ -1278,7 +1277,6 @@ virCgroupNewMachineManual(const char *name, if (virCgroupNewDomainPartition(parent, drivername, name, - true, &newGroup) < 0) return -1; @@ -2836,7 +2834,6 @@ int virCgroupNewDomainPartition(virCgroupPtr partition G_GNUC_UNUSED, const char *driver G_GNUC_UNUSED, const char *name G_GNUC_UNUSED, - bool create G_GNUC_UNUSED, virCgroupPtr *group G_GNUC_UNUSED) { virReportSystemError(ENXIO, "%s", diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index 7248bc1d35..77e01519b0 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -123,7 +123,6 @@ int virCgroupNewPartition(const char *path, int virCgroupNewDomainPartition(virCgroupPtr partition, const char *driver, const char *name, - bool create, virCgroupPtr *group) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index 0dd5e3f0b4..1baa71e61c 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -434,7 +434,7 @@ static int testCgroupNewForPartitionDomain(const void *args G_GNUC_UNUSED) return -1; } - if ((rv = virCgroupNewDomainPartition(partitioncgroup, "lxc", "foo", true, &domaincgroup)) != 0) { + if ((rv = virCgroupNewDomainPartition(partitioncgroup, "lxc", "foo", &domaincgroup)) != 0) { fprintf(stderr, "Cannot create LXC cgroup: %d\n", -rv); return -1; } @@ -475,7 +475,7 @@ static int testCgroupNewForPartitionDomainEscaped(const void *args G_GNUC_UNUSED return -1; } - if ((rv = virCgroupNewDomainPartition(partitioncgroup3, "lxc", "cpu.foo", true, &domaincgroup)) != 0) { + if ((rv = virCgroupNewDomainPartition(partitioncgroup3, "lxc", "cpu.foo", &domaincgroup)) != 0) { fprintf(stderr, "Cannot create LXC cgroup: %d\n", -rv); return -1; } -- 2.26.2

Use virStringSplit() to get the list of directories needed to be created. This improves readability of the code and stops passing absolute path to virCgroupNewFromParent(). Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroup.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 1e18b84b54..f4c1623567 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1131,21 +1131,18 @@ virCgroupEnableMissingControllers(char *path, virCgroupPtr *group) { g_autoptr(virCgroup) parent = NULL; - char *offset = path; + VIR_AUTOSTRINGLIST tokens = virStringSplit(path, "/", 0); + size_t i; - if (virCgroupNew("/", - controllers, - &parent) < 0) + if (virCgroupNew("/", controllers, &parent) < 0) return -1; - for (;;) { + /* Skip the first token as it is empty string. */ + for (i = 1; tokens[i]; i++) { g_autoptr(virCgroup) tmp = NULL; - char *t = strchr(offset + 1, '/'); - if (t) - *t = '\0'; if (virCgroupNewFromParent(parent, - path, + tokens[i], controllers, &tmp) < 0) return -1; @@ -1153,17 +1150,10 @@ virCgroupEnableMissingControllers(char *path, if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_SYSTEMD) < 0) return -1; - if (t) { - *t = '/'; - offset = t; - virCgroupFree(parent); - parent = g_steal_pointer(&tmp); - } else { - *group = g_steal_pointer(&tmp); - break; - } + parent = g_steal_pointer(&tmp); } + *group = g_steal_pointer(&parent); return 0; } -- 2.26.2

The old code passed an absolute path to virCgroupNewFromParent() which is not necessary. The code can take the current placement of parent cgroup and append a relative path. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/util/vircgroup.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 6caeb2d7f4..e0fe1dbf3e 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -896,6 +896,7 @@ virCgroupNewPartition(const char *path, g_autofree char *newPath = NULL; g_autoptr(virCgroup) parent = NULL; g_autoptr(virCgroup) newGroup = NULL; + char *partition = NULL; VIR_DEBUG("path=%s create=%d controllers=%x", path, create, controllers); @@ -914,17 +915,26 @@ virCgroupNewPartition(const char *path, if (STRNEQ(newPath, "/")) { char *tmp; - g_autofree char *parentPath = g_strdup(newPath); + const char *parentPath; - tmp = strrchr(parentPath, '/'); - tmp++; + tmp = strrchr(newPath, '/'); *tmp = '\0'; + if (tmp == newPath) { + parentPath = "/"; + } else { + parentPath = newPath; + } + if (virCgroupNew(parentPath, controllers, &parent) < 0) return -1; + + partition = tmp + 1; + } else { + partition = newPath; } - if (virCgroupNewFromParent(parent, newPath, controllers, &newGroup) < 0) + if (virCgroupNewFromParent(parent, partition, controllers, &newGroup) < 0) return -1; if (parent) { -- 2.26.2