Use automatic memory freeing for the temporary bitmap and remove the
pointless 'cleanup' section.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In some cases we have a label that contains nothing but a return
statement. The amount of such labels rises as we use automagic
cleanup. Anyway, such labels are pointless and can be dropped.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
There are a few cases where a string list is freed by an explicit
call of g_strfreev(), but the same result can be achieved by
g_atuo(GStrv).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
For new feature Fibre Channel VMID we will need to get inode of the
VM root cgroup as it is used in the new kernel API together with VMID.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
It all started as a simple bug: trying to move domain memory
between NUMA nodes (e.g. via virsh numatune) did not work. I've
traced the problem to qemuProcessHook() because that's where we
decide whether to rely on CGroups or use numactl APIs to satisfy
<numatune/>. The problem was that virCgroupControllerAvailable()
was telling us that cpuset controller is unavailable. This is
CGroupsV2, and pretty weird because CGroupsV2 definitely do
support cpuset controller and I had them mounted in a standard
way. What I found out (with Pavel's help) was that
virCgroupNewSelf() was looking into the following path to detect
supported controllers:
/sys/fs/cgroup/system.slice/cgroup.controllers
However, if there's no other VM running then the system.slice
only has 'memory' and 'pids' controllers. Therefore, we saw
'cpuset' as not available. The fix is to look at the top most
path, which has the full set of controllers:
/sys/fs/cgroup/cgroup.controllers
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1976690
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The g_path_is_absolute() considers more situations
than just a simply "path[0] == '/'".
Related issue: https://gitlab.com/libvirt/libvirt/-/issues/12
Signed-off-by: Luke Yue <lukedyue@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
I've encountered the following bug, but only on Gentoo with
systemd and CGroupsV2. I've started an LXC container successfully
but destroying it reported the following error:
error: Failed to destroy domain 'amd64'
error: internal error: failed to get cgroup backend for 'pathOfController'
Debugging showed, that CGroup hierarchy is full of surprises:
/sys/fs/cgroup/machine.slice/machine-lxc\x2d861\x2damd64.scope/
└── libvirt
├── dev-hugepages.mount
├── dev-mqueue.mount
├── init.scope
├── sys-fs-fuse-connections.mount
├── sys-kernel-config.mount
├── sys-kernel-debug.mount
├── sys-kernel-tracing.mount
├── system.slice
│ ├── console-getty.service
│ ├── dbus.service
│ ├── system-getty.slice
│ ├── system-modprobe.slice
│ ├── systemd-journald.service
│ ├── systemd-logind.service
│ └── tmp.mount
└── user.slice
For comparison, here's the same container on recent Rawhide:
/sys/fs/cgroup/machine.slice/machine-lxc\x2d13550\x2damd64.scope/
└── libvirt
Anyway, those nested directories should not be a problem, because
virCgroupKillRecursiveInternal() removes them recursively, right?
Sort of. The function really does remove nested directories, but
it assumes that every directory has the same controller as the
rest. Just take a look at virCgroupV2KillRecursive() - it gets
'Any' controller (the first one it found in ".scope") and then
passes it to virCgroupKillRecursiveInternal().
This assumption is not true though. The controllers found in
".scope" are the following:
cpuset cpu io memory pids
while "libvirt" has fewer:
cpuset cpu io memory
Up until now it's not problem, because of how we order
controllers internally - "cpu" is the first and thus picking
"Any" controller returns just that. But the rest of directories
has no controllers, their "cgroup.controllers" is just empty.
What fixes the bug is dropping @controller argument from
virCgroupKillRecursiveInternal() and letting each iteration work
pick its own controller.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Currently, only a subset of virCgroupKillRecursiveInternal()
arguments is printed into debug logs. Print all of them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Historically, we declared pointer type to our types:
typedef struct _virXXX virXXX;
typedef virXXX *virXXXPtr;
But usefulness of such declaration is questionable, at best.
Unfortunately, we can't drop every such declaration - we have to
carry some over, because they are part of public API (e.g.
virDomainPtr). But for internal types - we can do drop them and
use what every other C project uses 'virXXX *'.
This change was generated by a very ugly shell script that
generated sed script which was then called over each file in the
repository. For the shell script refer to the cover letter:
https://listman.redhat.com/archives/libvir-list/2021-March/msg00537.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Previous commit removed all usage of this function so we can remove it.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Our implementation was inspired by glib anyways. The difference is only
the order of arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Our implementation was heavily inspired by the glib version so it's a
drop-in replacement.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Lookup the string with prefix locally so that we can remove the helper
which isn't universal at all.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When running on host with systemd we register VMs with machined.
In this case systemd creates the root VM cgroup for us. This has some
implications where one of them is that systemd owns all files inside
the root VM cgroup and we should not touch them.
We already use DBus calls for some of the APIs but for the remaining
ones we will continue accessing the files directly. Systemd doesn't
support threaded cgroups so we need to do this.
The reason why we don't use DBus for most of the APIs is that we already
have a code that works with files and we would have to check if systemd
supports each API.
This change introduces new topology on systemd hosts:
$ROOT
|
+- machine.slice
|
+- machine-qemu\x2d1\x2dvm1.scope
|
+- libvirt
|
+- emulator
+- vcpu0
+- vcpu0
compared to the previous topology:
$ROOT
|
+- machine.slice
|
+- machine-qemu\x2d1\x2dvm1.scope
|
+- emulator
+- vcpu0
+- vcpu0
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When we create a new child cgroup and the parent cgroup has any process
attached to it enabling controllers for the child cgroup fails with
error. We need to move the process into the child cgroup first before
enabling any controllers.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When running on host with systemd we register VMs with machined.
In this case systemd creates the root VM cgroup for us. This has some
implications where one of them is that systemd owns all files inside
the root VM cgroup and we should not touch them.
If we change any value in file that systemd knows about it will be
changed to what systemd thinks it should be when executing
`systemctl daemon-reload`.
These are the APIs that we need to call using systemd because they set
limits that are proportional to sibling cgroups.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
A memory leak was identified in
virCgroupEnableMissingControllers():
==11680== at 0x483EAE5: calloc (vg_replace_malloc.c:760)
==11680== by 0x4E51780: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6701.0)
==11680== by 0x4908618: virCgroupNew (vircgroup.c:701)
==11680== by 0x49096F4: virCgroupEnableMissingControllers (vircgroup.c:1146)
==11680== by 0x4909B17: virCgroupNewMachineSystemd (vircgroup.c:1228)
==11680== by 0x4909E94: virCgroupNewMachine (vircgroup.c:1313)
==11680== by 0x1694FDBC: qemuInitCgroup (qemu_cgroup.c:946)
==11680== by 0x1695046B: qemuSetupCgroup (qemu_cgroup.c:1083)
==11680== by 0x16A60126: qemuProcessLaunch (qemu_process.c:7077)
==11680== by 0x16A61504: qemuProcessStart (qemu_process.c:7384)
==11680== by 0x169B84C2: qemuDomainObjStart (qemu_driver.c:6590)
==11680== by 0x169B8776: qemuDomainCreateWithFlags (qemu_driver.c:6641)
What happens is that new virCgroup is created and stored into
@parent. Then, if @tokens is not empty the for() loop is entered
into where another virCgroup is created and @parent is replaced
with this new virCgroup. But nothing freed the old @parent.
Fixes: 77291414c7
Reported-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Glib provides g_auto(GStrv) which is in-place replacement of our
VIR_AUTOSTRINGLIST.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
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@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
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@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Currently this task is done by virCgroupCopyPlacement when the @path
starts with "/".
virCgroupNew is always called with @path starting with "/" and there is
no parent to copy path from. To make it obvious what the code is doing
introduce new helper.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This function is relevant only with cgroups v1 where it creates
hierarchy for controllers that are not managed by systemd. PID is used
to detect a placement of current process but in this situation we are
building the hierarchy for already known placement.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
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@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
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@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
It is only used for debug and error purposes which can be easily
replaced by @placement.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
After converting all DIR* to g_autoptr(DIR), many cleanup: labels
ended up just having "return ret", and every place that set ret would
just immediately goto cleanup. Remove the cleanup label and its
return, and just return the set value immediately, thus eliminating
the need for the return variable itself.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
All of these conversions are trivial - VIR_DIR_CLOSE() (aka
virDirClose()) is called only once on the DIR*, and it happens just
before going out of scope.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Remove 'cleanup' label and simplify remembering of the returned value
from the callback.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Rewrite using GHashTable which already has interfaces for using a number
as hash key. Glib's implementation doesn't copy the key by default, so
we need to allocate it, but overal the interface is more suited for this
case.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
virCgroupKillRecursive sneakily initializes 'ret' to 0 rather than the
usual -1. 401030499b moved an error condition but didn't actually
modify 'ret' return the proper error code.
Fixes: 401030499b
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
As preparation for g_autoptr() we need to change the function to take
only virCgroupPtr.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Both accept a NULL value gracefully and virStringFreeList
does not zero the pointer afterwards, so a straight replace
is safe.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
