[PATCH] qemu_extdevice: Add a comment into qemuExtDevicesSetupCgroup()

Michal Privoznik posted 1 patch 1 year, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/5eb7cfbdc5e3f243a7274f7e53110fcc8f4e2967.1676474571.git.mprivozn@redhat.com
src/qemu/qemu_extdevice.c | 3 +++
1 file changed, 3 insertions(+)
[PATCH] qemu_extdevice: Add a comment into qemuExtDevicesSetupCgroup()
Posted by Michal Privoznik 1 year, 1 month ago
The way setting up CGroups for external helpers work, is:
qemuExtDevicesHasDevice() is called first to determine whether
there is a helper process running, the CGroup controller is
created and then qemuExtDevicesSetupCgroup() is called to place
helpers into the CGroup. But when one reads just
qemuExtDevicesSetupCgroup() it's easy to miss this hidden logic.
Therefore, add a warning at the beginning of the function.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_extdevice.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index 47e97f3565..1c397972e4 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -331,6 +331,9 @@ qemuExtDevicesSetupCgroup(virQEMUDriver *driver,
     virDomainDef *def = vm->def;
     size_t i;
 
+    /* Don't forget to adjust qemuExtDevicesHasDevice() accordingly.
+     * Otherwise, this function might not be called at all. */
+
     if (qemuDBusSetupCgroup(driver, vm, cgroup) < 0)
         return -1;
 
-- 
2.39.1
Re: [PATCH] qemu_extdevice: Add a comment into qemuExtDevicesSetupCgroup()
Posted by Laine Stump 1 year, 1 month ago
On 2/15/23 10:22 AM, Michal Privoznik wrote:
> The way setting up CGroups for external helpers work, is:
> qemuExtDevicesHasDevice() is called first to determine whether
> there is a helper process running, the CGroup controller is
> created and then qemuExtDevicesSetupCgroup() is called to place
> helpers into the CGroup. But when one reads just
> qemuExtDevicesSetupCgroup() it's easy to miss this hidden logic.
> Therefore, add a warning at the beginning of the function.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

Thanks for indulging me!

> ---
>   src/qemu/qemu_extdevice.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> index 47e97f3565..1c397972e4 100644
> --- a/src/qemu/qemu_extdevice.c
> +++ b/src/qemu/qemu_extdevice.c
> @@ -331,6 +331,9 @@ qemuExtDevicesSetupCgroup(virQEMUDriver *driver,
>       virDomainDef *def = vm->def;
>       size_t i;
>   
> +    /* Don't forget to adjust qemuExtDevicesHasDevice() accordingly.
> +     * Otherwise, this function might not be called at all. */
> +
>       if (qemuDBusSetupCgroup(driver, vm, cgroup) < 0)
>           return -1;
>