On Tue, 2016-04-05 at 17:32 +0200, Ján Tomko wrote: > On Tue, Apr 05, 2016 at 03:08:41PM +0200, Andrea Bolognani wrote: > > The existing code was built with the assumption that no cgroup > > name could appear as part of another cgroup name. > > This is not true - the code checked if there is a comma or a null > character after the cgroup name. But not whether there was a comma *before* the cgroup name. So an ipotetic cgroup called 'fakecpu' would match when looking for the 'cpu' cgroup. More importantly, only the first match is taken into account: on my system, the 'cpu' and 'cpuacct' are mounted to the same location and the corresponding mtab entry looks like cgroup /sys/fs/cgroup/cpu,cpuacct \ cgroup rw,nosuid,nodev,noexec,relatime,cpu,cpuacct 0 0 but it's been reported that, on other systems, the 'cpuacct' mount flag is listed *before* the 'cpu' mount flag. When that's the case, strstr() will match the first occurrence of 'cpu', which happens to be that at the beginning of 'cpuacct', and the subsequent check for that match be followed by a comma will fail. To sum up: the commit message is not good enough and should be improved, but the change itself (or a variation thereof) is still needed to make the cgroup mount point detection robust. > > + /* Ignore non-cgroup mounts */ > > + if (STRNEQ(ent.mnt_type, "cgroup")) > > continue; > > > > This hunk makes sense, but I think it is unlikely to have a mount option > matching a cgroup name on a non-cgroup mount. Yeah, I tend to agree. Still, better safe than sorry, right? And we can avoid calling virStringSplit() if we know we're not processing a cgroup mount. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list