Re: [PATCH] qemu_domain: Check if driver->domainEventState is NULL

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

On 6/20/24 11:21, Rayhan Faizel wrote:
> Under the test environment, driver->domainEventState is uninitialized. If a
> disk gets dropped, it will attempt to queue an event which will cause a
> segmentation fault. This crash does not occur during normal use.
> 
> This patch adds a quick check to ensure driver->domainEventState is not NULL
> along with a testcase exercising the dropping of disks with startupPolicy set
> as 'optional'.
> 
> Signed-off-by: Rayhan Faizel <rayhan.faizel@xxxxxxxxx>
> ---
>  src/qemu/qemu_domain.c                        |  3 +-
>  ...tuppolicy-optional-drop.x86_64-latest.args | 33 ++++++++++++++++
>  ...rtuppolicy-optional-drop.x86_64-latest.xml | 38 +++++++++++++++++++
>  .../disk-startuppolicy-optional-drop.xml      | 23 +++++++++++
>  tests/qemuxmlconftest.c                       |  2 +
>  5 files changed, 98 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.args
>  create mode 100644 tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.xml
>  create mode 100644 tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.xml
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 7ba2ea4a5e..109c5bbd52 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7592,7 +7592,8 @@ qemuDomainCheckRemoveOptionalDisk(virQEMUDriver *driver,
>          virDomainDiskDefFree(disk);
>      }
>  
> -    virObjectEventStateQueue(driver->domainEventState, event);
> +    if (driver->domainEventState)
> +        virObjectEventStateQueue(driver->domainEventState, event);
>  }

I'd rather see our tests creating this queue. E.g. like this:

diff --git i/tests/testutilsqemu.c w/tests/testutilsqemu.c
index d70850cb5d..b870657063 100644
--- i/tests/testutilsqemu.c
+++ w/tests/testutilsqemu.c
@@ -231,6 +231,7 @@ void qemuTestDriverFree(virQEMUDriver *driver)
         virFileDeleteTree(driver->config->stateDir);
         virFileDeleteTree(driver->config->configDir);
     }
+    virObjectUnref(driver->domainEventState);
     virObjectUnref(driver->qemuCapsCache);
     virObjectUnref(driver->xmlopt);
     virObjectUnref(driver->caps);
@@ -343,6 +344,9 @@ int qemuTestDriverInit(virQEMUDriver *driver)
 
     cfg->configDir = g_strdup(configdir);
 
+    if (!(driver->domainEventState = virObjectEventStateNew()))
+        goto error;
+
     driver->caps = testQemuCapsInit();
     if (!driver->caps)
         goto error;

BTW: this is also what qemuhotplugtest does:

diff --git i/tests/qemuhotplugtest.c w/tests/qemuhotplugtest.c
index d935ad58ed..f707121c47 100644
--- i/tests/qemuhotplugtest.c
+++ w/tests/qemuhotplugtest.c
@@ -501,9 +501,6 @@ mymain(void)
 
     virEventRegisterDefaultImpl();
 
-    if (!(driver.domainEventState = virObjectEventStateNew()))
-        return EXIT_FAILURE;
-
     driver.lockManager = virLockManagerPluginNew("nop", "qemu",
                                                  driver.config->configBaseDir,
                                                  0);


Michal