[PATCH 0/2] qemuxml2argvtest: Don't exec '/usr/libexec/qemu/vhost-user/test-vhost-user-gpu'
'qemuExtVhostUserGPUPrepareDomain' breaks our design assumptions about the 'PrepareDomain' step which should not touch anything on the host. This patchset for now fixes the symptom by mocking the function and poisons virFork and virCommandRun so that this doesn't happen in the future. Proper fix will require splitting the vhost-user GPU prepare step to prepare the host-specific portion separately. Peter Krempa (2): qemuxml2argvmock: Mock qemuExtVhostUserGPUPrepareDomain qemuxml2argvmock: Poison virCommandRun and virFork from test context src/qemu/qemu_vhost_user_gpu.c | 4 ++++ src/qemu/qemu_vhost_user_gpu.h | 2 +- src/util/vircommand.h | 4 ++-- tests/qemuxml2argvmock.c | 28 ++++++++++++++++++++++++++++ 4 files changed, 35 insertions(+), 3 deletions(-) -- 2.43.0
Noticed when breaking on 'fork()' in qemuxml2argvtest (output slihtly trimmed, of unimportant file locations: #0 0x00007ffff75abf18 in fork () at /lib64/libc.so.6 #1 0x00007ffff7b88387 in virFork () at ../../../libvirt/src/util/vircommand.c:282 #2 0x00007ffff7b8a7d2 in virExec (cmd=0x55555740d440) at ../../../libvirt/src/util/vircommand.c:741 #3 virCommandRunAsync (cmd=cmd@entry=0x55555740d440, pid=pid@entry=0x0) at ../../../libvirt/src/util/vircommand.c:2658 #4 0x00007ffff7b8c04f in virCommandRun (cmd=cmd@entry=0x55555740d440, exitstatus=exitstatus@entry=0x0) #5 0x00007ffff79f6daa in qemuVhostUserFillDomainGPU (driver=driver@entry=0x555555586120 <driver>, video=0x5555573e8d80) #6 0x00007ffff79f73f5 in qemuExtVhostUserGPUPrepareDomain (driver=driver@entry=0x555555586120 <driver>, video=<optimized out>) #7 0x00007ffff797c569 in qemuExtDevicesPrepareDomain (driver=driver@entry=0x555555586120 <driver>, vm=vm@entry=0x5555573dfef0) #8 0x00007ffff79d69ef in qemuProcessPrepareDomain #9 0x00007ffff79dda36 in qemuProcessCreatePretendCmdPrepare #10 0x000055555556fa28 in testCompareXMLToArgvCreateArgs #11 testCompareXMLToArgv (data=0x5555573440f0) at ../../../libvirt/tests/qemuxml2argvtest.c:733 #12 0x0000555555570f7a in virTestRun #13 0x0000555555571201 in virTestRunLog (ret=0x7fffffffdb4c, title=0x55555697c360 "QEMU XML-2-ARGV virtio-options.x86_64-latest", body=0x55555556f820 <testCompareXMLToArgv>, data=0x5555573440f0) at ../../../libvirt/tests/testutils.c:198 #14 0x000055555555c1b9 in testRun Code paths in 'qemuProcessPrepareDomain' should not invoke external helpers. Note this in a comment and mock the function for now. It will need a more complex refactor. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_vhost_user_gpu.c | 4 ++++ src/qemu/qemu_vhost_user_gpu.h | 2 +- tests/qemuxml2argvmock.c | 9 +++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c index 933adfe8de..cca76526d1 100644 --- a/src/qemu/qemu_vhost_user_gpu.c +++ b/src/qemu/qemu_vhost_user_gpu.c @@ -78,6 +78,10 @@ qemuVhostUserGPUGetPid(const char *stateDir, } +/** TODO: this is called from qemuProcessPrepareDomain which is NOT supposed to + * query the host in any way. This function is mocked in qemuxml2argvmock.so + * to prevent probing the host vgpu process capabilities. + */ int qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver, virDomainVideoDef *video) { diff --git a/src/qemu/qemu_vhost_user_gpu.h b/src/qemu/qemu_vhost_user_gpu.h index 2b86982cb8..d19798d781 100644 --- a/src/qemu/qemu_vhost_user_gpu.h +++ b/src/qemu/qemu_vhost_user_gpu.h @@ -26,7 +26,7 @@ int qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver, virDomainVideoDef *video) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) - G_GNUC_WARN_UNUSED_RESULT; + G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE; int qemuExtVhostUserGPUStart(virQEMUDriver *driver, virDomainObj *vm, diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 52c44b2ed0..2deccd79c4 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -37,6 +37,7 @@ #include "qemu/qemu_command.h" #include <unistd.h> #include <fcntl.h> +#include "qemu/qemu_vhost_user_gpu.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -267,3 +268,11 @@ virIdentityEnsureSystemToken(void) { return g_strdup("3de80bcbf22d4833897f1638e01be9b2"); } + + +int +qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver G_GNUC_UNUSED, + virDomainVideoDef *video G_GNUC_UNUSED) +{ + return 0; +} -- 2.43.0
We don't want to ever run an actuall command in qemuxml2argvtest poison the helper functions we have for running commands. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.h | 4 ++-- tests/qemuxml2argvmock.c | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 9bcdce35b9..566ac15947 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -39,7 +39,7 @@ typedef struct _virCommand virCommand; * call any function that is not async-signal-safe. */ typedef int (*virExecHook)(void *data); -pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT; +pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE; virCommand *virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1); @@ -184,7 +184,7 @@ int virCommandGetArgList(virCommand *cmd, char ***args); int virCommandExec(virCommand *cmd, gid_t *groups, int ngroups) G_GNUC_WARN_UNUSED_RESULT; int virCommandRun(virCommand *cmd, - int *exitstatus) G_GNUC_WARN_UNUSED_RESULT; + int *exitstatus) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE; int virCommandRunAsync(virCommand *cmd, pid_t *pid) G_GNUC_WARN_UNUSED_RESULT; diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 2deccd79c4..0ee8fbba79 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -276,3 +276,22 @@ qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver G_GNUC_UNUSED, { return 0; } + + +int +virCommandRun(virCommand *cmd, + int *exitstatus G_GNUC_UNUSED) +{ + const char *path = virCommandGetBinaryPath(cmd); + + fprintf(stderr, "\nattempted virCommandRun() (path=%s) from test context\n", NULLSTR(path)); + return -1; +} + + +pid_t +virFork(void) +{ + fprintf(stderr, "\nattempted virFork() in test context\n"); + return -1; +} -- 2.43.0
On 12/18/23 12:02, Peter Krempa wrote:
We don't want to ever run an actuall command in qemuxml2argvtest poison the helper functions we have for running commands.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.h | 4 ++-- tests/qemuxml2argvmock.c | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 9bcdce35b9..566ac15947 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -39,7 +39,7 @@ typedef struct _virCommand virCommand; * call any function that is not async-signal-safe. */ typedef int (*virExecHook)(void *data);
-pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT; +pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
virCommand *virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1);
@@ -184,7 +184,7 @@ int virCommandGetArgList(virCommand *cmd, char ***args); int virCommandExec(virCommand *cmd, gid_t *groups, int ngroups) G_GNUC_WARN_UNUSED_RESULT;
int virCommandRun(virCommand *cmd, - int *exitstatus) G_GNUC_WARN_UNUSED_RESULT; + int *exitstatus) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
int virCommandRunAsync(virCommand *cmd, pid_t *pid) G_GNUC_WARN_UNUSED_RESULT; diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 2deccd79c4..0ee8fbba79 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -276,3 +276,22 @@ qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver G_GNUC_UNUSED, { return 0; } + + +int +virCommandRun(virCommand *cmd, + int *exitstatus G_GNUC_UNUSED) +{ + const char *path = virCommandGetBinaryPath(cmd); + + fprintf(stderr, "\nattempted virCommandRun() (path=%s) from test context\n", NULLSTR(path)); + return -1; +} + + +pid_t +virFork(void) +{ + fprintf(stderr, "\nattempted virFork() in test context\n"); + return -1; +}
Any reason to mock both? Or is it just to provide nicer error message? Michal
On Mon, Dec 18, 2023 at 15:17:01 +0100, Michal Prívozník wrote:
On 12/18/23 12:02, Peter Krempa wrote:
We don't want to ever run an actuall command in qemuxml2argvtest poison the helper functions we have for running commands.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.h | 4 ++-- tests/qemuxml2argvmock.c | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 9bcdce35b9..566ac15947 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -39,7 +39,7 @@ typedef struct _virCommand virCommand; * call any function that is not async-signal-safe. */ typedef int (*virExecHook)(void *data);
-pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT; +pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
virCommand *virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1);
@@ -184,7 +184,7 @@ int virCommandGetArgList(virCommand *cmd, char ***args); int virCommandExec(virCommand *cmd, gid_t *groups, int ngroups) G_GNUC_WARN_UNUSED_RESULT;
int virCommandRun(virCommand *cmd, - int *exitstatus) G_GNUC_WARN_UNUSED_RESULT; + int *exitstatus) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
int virCommandRunAsync(virCommand *cmd, pid_t *pid) G_GNUC_WARN_UNUSED_RESULT; diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 2deccd79c4..0ee8fbba79 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -276,3 +276,22 @@ qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver G_GNUC_UNUSED, { return 0; } + + +int +virCommandRun(virCommand *cmd, + int *exitstatus G_GNUC_UNUSED) +{ + const char *path = virCommandGetBinaryPath(cmd); + + fprintf(stderr, "\nattempted virCommandRun() (path=%s) from test context\n", NULLSTR(path)); + return -1; +} + + +pid_t +virFork(void) +{ + fprintf(stderr, "\nattempted virFork() in test context\n"); + return -1; +}
Any reason to mock both? Or is it just to provide nicer error message?
Reasonable error message is reported already by virCommandRun. I just wanted to make sure that the test doesn't try to even fork for any other reason, thus I've picked our other helper to prevent that too (IIRC plain fork() is forbidden by syntax-check). For catching the mistake fixed in 1/2 only the mock of virCommandRun is necessary.
On 12/18/23 15:26, Peter Krempa wrote:
On Mon, Dec 18, 2023 at 15:17:01 +0100, Michal Prívozník wrote:
On 12/18/23 12:02, Peter Krempa wrote:
We don't want to ever run an actuall command in qemuxml2argvtest poison the helper functions we have for running commands.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.h | 4 ++-- tests/qemuxml2argvmock.c | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 9bcdce35b9..566ac15947 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -39,7 +39,7 @@ typedef struct _virCommand virCommand; * call any function that is not async-signal-safe. */ typedef int (*virExecHook)(void *data);
-pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT; +pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
virCommand *virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1);
@@ -184,7 +184,7 @@ int virCommandGetArgList(virCommand *cmd, char ***args); int virCommandExec(virCommand *cmd, gid_t *groups, int ngroups) G_GNUC_WARN_UNUSED_RESULT;
int virCommandRun(virCommand *cmd, - int *exitstatus) G_GNUC_WARN_UNUSED_RESULT; + int *exitstatus) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
int virCommandRunAsync(virCommand *cmd, pid_t *pid) G_GNUC_WARN_UNUSED_RESULT; diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 2deccd79c4..0ee8fbba79 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -276,3 +276,22 @@ qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver G_GNUC_UNUSED, { return 0; } + + +int +virCommandRun(virCommand *cmd, + int *exitstatus G_GNUC_UNUSED) +{ + const char *path = virCommandGetBinaryPath(cmd); + + fprintf(stderr, "\nattempted virCommandRun() (path=%s) from test context\n", NULLSTR(path)); + return -1; +} + + +pid_t +virFork(void) +{ + fprintf(stderr, "\nattempted virFork() in test context\n"); + return -1; +}
Any reason to mock both? Or is it just to provide nicer error message?
Reasonable error message is reported already by virCommandRun. I just wanted to make sure that the test doesn't try to even fork for any other reason, thus I've picked our other helper to prevent that too (IIRC plain fork() is forbidden by syntax-check).
Yeah, we have a syntax-check to enforce virFork(), so mocking just virFork() should be fine. In the end, virCommandRun() does call virFork(). I don't mind and don't want to bikeshed. Feel free to push your patch as is. I was just curious. Michal
On 12/18/23 12:02, Peter Krempa wrote:
'qemuExtVhostUserGPUPrepareDomain' breaks our design assumptions about the 'PrepareDomain' step which should not touch anything on the host.
This patchset for now fixes the symptom by mocking the function and poisons virFork and virCommandRun so that this doesn't happen in the future.
Proper fix will require splitting the vhost-user GPU prepare step to prepare the host-specific portion separately.
Peter Krempa (2): qemuxml2argvmock: Mock qemuExtVhostUserGPUPrepareDomain qemuxml2argvmock: Poison virCommandRun and virFork from test context
src/qemu/qemu_vhost_user_gpu.c | 4 ++++ src/qemu/qemu_vhost_user_gpu.h | 2 +- src/util/vircommand.h | 4 ++-- tests/qemuxml2argvmock.c | 28 ++++++++++++++++++++++++++++ 4 files changed, 35 insertions(+), 3 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On 12/18/23 15:17, Michal Prívozník wrote:
On 12/18/23 12:02, Peter Krempa wrote:
'qemuExtVhostUserGPUPrepareDomain' breaks our design assumptions about the 'PrepareDomain' step which should not touch anything on the host.
This patchset for now fixes the symptom by mocking the function and poisons virFork and virCommandRun so that this doesn't happen in the future.
Proper fix will require splitting the vhost-user GPU prepare step to prepare the host-specific portion separately.
Peter Krempa (2): qemuxml2argvmock: Mock qemuExtVhostUserGPUPrepareDomain qemuxml2argvmock: Poison virCommandRun and virFork from test context
src/qemu/qemu_vhost_user_gpu.c | 4 ++++ src/qemu/qemu_vhost_user_gpu.h | 2 +- src/util/vircommand.h | 4 ++-- tests/qemuxml2argvmock.c | 28 ++++++++++++++++++++++++++++ 4 files changed, 35 insertions(+), 3 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
I was wondering why 'meson test --setup acccess' did not catch this and there are two reasons: 1) it doesn't check for exec(), 2) because of filewrapper, it's not really /usr/libexec/... we are trying to exec, rather than libvirt.git/tests/qemuvhostuserdata/usr/libexec/qemu/vhost-user/test-vhost-user-gpu which is just a shell script. I'll post patch for 1), might be worth removing the script from 2) too. Michal
participants (2)
-
Michal Prívozník -
Peter Krempa