[PATCH] qemu: fix locking in qemuProcessHandleMemoryFailure

Thomas Prescher posted 1 patch 3 weeks, 4 days ago
Failed in applying to current master (apply log)
There is a newer version of this series
src/qemu/qemu_process.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
[PATCH] qemu: fix locking in qemuProcessHandleMemoryFailure
Posted by Thomas Prescher 3 weeks, 4 days ago
Fix locking in qemuProcessHandleMemoryFailure. We use a lock guard
now because we can directly return from the default switch cases.

Issue has been discovered by johannes.kulik@sap.com

On-behalf-of: SAP thomas.prescher@sap.com
Signed-off-by: Thomas Prescher <thomas.prescher@cyberus-technology.de>
---
 src/qemu/qemu_process.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4e1d713809..cc902e1d37 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1940,7 +1940,7 @@ qemuProcessHandleMemoryFailure(qemuMonitor *mon G_GNUC_UNUSED,
     virDomainMemoryFailureActionType action;
     unsigned int flags = 0;
 
-    virObjectLock(vm);
+    VIR_LOCK_GUARD lock = virObjectLockGuard(vm);
     driver = QEMU_DOMAIN_PRIVATE(vm)->driver;
 
     switch (mfp->recipient) {
@@ -1980,8 +1980,6 @@ qemuProcessHandleMemoryFailure(qemuMonitor *mon G_GNUC_UNUSED,
 
     event = virDomainEventMemoryFailureNewFromObj(vm, recipient, action, flags);
 
-    virObjectUnlock(vm);
-
     virObjectEventStateQueue(driver->domainEventState, event);
 }
 
-- 
2.52.0
Re: [PATCH] qemu: fix locking in qemuProcessHandleMemoryFailure
Posted by Michal Prívozník via Devel 3 weeks, 3 days ago
On 1/7/26 15:38, Thomas Prescher wrote:
> Fix locking in qemuProcessHandleMemoryFailure. We use a lock guard
> now because we can directly return from the default switch cases.

Yeah, this doesn't look good but IIUC that can never happen. I mean,
qemuMonitorJSONHandleMemoryFailure() parses the incoming event and if
there's an unknown recipient or action then the function throws a
warning and returns early. Nevertheless, the pattern as-is gives a bad
example.

> 
> Issue has been discovered by johannes.kulik@sap.com
> 
> On-behalf-of: SAP thomas.prescher@sap.com
> Signed-off-by: Thomas Prescher <thomas.prescher@cyberus-technology.de>
> ---
>  src/qemu/qemu_process.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 4e1d713809..cc902e1d37 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1940,7 +1940,7 @@ qemuProcessHandleMemoryFailure(qemuMonitor *mon G_GNUC_UNUSED,
>      virDomainMemoryFailureActionType action;
>      unsigned int flags = 0;
>  
> -    virObjectLock(vm);
> +    VIR_LOCK_GUARD lock = virObjectLockGuard(vm);
>      driver = QEMU_DOMAIN_PRIVATE(vm)->driver;

Nit pick, the 'lock' variable declaration should go into the block
that's declaring other variables.

>  
>      switch (mfp->recipient) {
> @@ -1980,8 +1980,6 @@ qemuProcessHandleMemoryFailure(qemuMonitor *mon G_GNUC_UNUSED,
>  
>      event = virDomainEventMemoryFailureNewFromObj(vm, recipient, action, flags);
>  
> -    virObjectUnlock(vm);
> -
>      virObjectEventStateQueue(driver->domainEventState, event);
>  }
>  

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal