[libvirt] [PATCH v2 0/2] fix segfaults on qemuhotplugtest
After debugging for a while and eventually finding the root cause we discussed this on IRC today: <cpaelzer> danpb: BTW the test crash in 6.0 that I'm tracing (asked yesterday) is virHostdevManagerInitialize failing <cpaelzer> the test goes on with mgr set to 0x0 and that is the segfault later on <cpaelzer> virSetError was helpful as "Could not initialize HostdevManager operation failed: Failed to create state dir '/sbuild-nonexistent/.cache/libvirt/hostdevmgr'" sounds like a very testbed-specific issue that I might be able to solve when invoking the tests <danpb> hmm, seems like we're missing a mock for mkdir() <danpb> i guess we should mock virFileMakePath in this case This series got v6.0.0 building in Ubuntu, so I thought it is worth to submit it for review. Updates in v2: - improve indent of virGetLastErrorMessage - drop superfluous curly brackets - added the Reviewed-by of Daniel Henrique Barboza (Thanks !) Christian Ehrhardt (2): test: let qemuhotplugtest report details of init fails test: qemuhotplugtest mock virFileMakePath tests/qemuhotplugmock.c | 17 +++++++++++++++++ tests/qemuhotplugtest.c | 5 +++++ 2 files changed, 22 insertions(+) -- 2.25.0
If virHostdevManagerGetDefault in qemuhotplugtest fails it works for quite a while to later segfault when accessing mgr->activePCIHostdevs. Report the error details and break on a failed init to see the real issue right away. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- tests/qemuhotplugtest.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index a60c8d1c93..6a3e61c54b 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -634,6 +634,11 @@ mymain(void) return EXIT_FAILURE; driver.hostdevMgr = virHostdevManagerGetDefault(); + if (driver.hostdevMgr == NULL) { + VIR_TEST_VERBOSE("Could not initialize HostdevManager - %s\n", + virGetLastErrorMessage()); + return EXIT_FAILURE; + } #define DO_TEST(file, ACTION, dev, fial, kep, ...) \ -- 2.25.0
On Thu, 2020-01-16 at 09:28 +0100, Christian Ehrhardt wrote:
If virHostdevManagerGetDefault in qemuhotplugtest fails it works for quite a while to later segfault when accessing mgr->activePCIHostdevs.
Report the error details and break on a failed init to see the real issue right away.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- tests/qemuhotplugtest.c | 5 +++++ 1 file changed, 5 insertions(+)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jan 17, 2020 at 6:32 PM Andrea Bolognani <abologna@redhat.com> wrote:
On Thu, 2020-01-16 at 09:28 +0100, Christian Ehrhardt wrote:
If virHostdevManagerGetDefault in qemuhotplugtest fails it works for quite a while to later segfault when accessing mgr->activePCIHostdevs.
Report the error details and break on a failed init to see the real issue right away.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- tests/qemuhotplugtest.c | 5 +++++ 1 file changed, 5 insertions(+)
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks for the reviews Daniel and Andrea, I have pushed this (just this - not the 2/2 which is still up for discussion) to master now.
-- Andrea Bolognani / Red Hat / Virtualization
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
In certain build environments (e.g. Debian and Ubuntu) $HOME is set to a non existing path intentionally. That breaks and crashes qemuhotplugtest by failing the init in virHostdevManagerGetDefault. Avoid that issue by mocking the virFileMakePath behavior if it is passed a path that matches $HOME and doesn't exists. That fixes qemuhotplugtest in Debian/Ubuntu builds of v6.0.0. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- tests/qemuhotplugmock.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c index 43a9d79051..06a28742d4 100644 --- a/tests/qemuhotplugmock.c +++ b/tests/qemuhotplugmock.c @@ -18,6 +18,7 @@ #include <config.h> +#include "virmock.h" #include "qemu/qemu_hotplug.h" #include "conf/domain_conf.h" @@ -31,3 +32,19 @@ qemuDomainGetUnplugTimeout(virDomainObjPtr vm G_GNUC_UNUSED) return 200; return 100; } + +VIR_MOCK_IMPL_RET_ARGS(virFileMakePath, int, + const char *, path) +{ + const char *home; + + VIR_MOCK_REAL_INIT(virFileMakePath); + + /* ignore non-existing homes (e.g. in build environments) */ + home = getenv("HOME"); + if (strstr(path, home)) { + if (!g_file_test (home, G_FILE_TEST_EXISTS)) + return 0; + } + return real_virFileMakePath(path); +} -- 2.25.0
On Thu, 2020-01-16 at 09:28 +0100, Christian Ehrhardt wrote:
+VIR_MOCK_IMPL_RET_ARGS(virFileMakePath, int, + const char *, path) +{ + const char *home; + + VIR_MOCK_REAL_INIT(virFileMakePath); + + /* ignore non-existing homes (e.g. in build environments) */ + home = getenv("HOME"); + if (strstr(path, home)) { + if (!g_file_test (home, G_FILE_TEST_EXISTS)) + return 0; + } + return real_virFileMakePath(path); +}
This doesn't look like the correct fix: what will happen is that, instead of creating the directory the library code expects, we will not create it and lie to the caller about this fact. What we should do instead is create the directory, but make sure it is prefixed with LIBVIRT_FAKE_ROOT_DIR, in a similar way to what's done in virpcimock. Of course file access will need to be mocked in the same way for the tests to work... -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jan 17, 2020 at 6:47 PM Andrea Bolognani <abologna@redhat.com> wrote:
On Thu, 2020-01-16 at 09:28 +0100, Christian Ehrhardt wrote:
+VIR_MOCK_IMPL_RET_ARGS(virFileMakePath, int, + const char *, path) +{ + const char *home; + + VIR_MOCK_REAL_INIT(virFileMakePath); + + /* ignore non-existing homes (e.g. in build environments) */ + home = getenv("HOME"); + if (strstr(path, home)) { + if (!g_file_test (home, G_FILE_TEST_EXISTS)) + return 0; + } + return real_virFileMakePath(path); +}
This doesn't look like the correct fix: what will happen is that, instead of creating the directory the library code expects, we will not create it and lie to the caller about this fact.
Not creating and lying was exactly what I had in mind for this particular case as it was the least invasive change to achieve what was needed for the test. What we should do instead is create the directory, but make sure it
is prefixed with LIBVIRT_FAKE_ROOT_DIR, in a similar way to what's done in virpcimock. Of course file access will need to be mocked in the same way for the tests to work...
I was initially adding all sorts of virFileWrapperAddPrefix but it failed me. Trying again in the style of virpcimock seems like a good idea, but I have to be honest due to a business trip and some other tasks I won't get to it soon. If that build error with non-existing $HOME bothers anyone else feel free to beat me to it with a v3 of this. --
Andrea Bolognani / Red Hat / Virtualization
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
participants (2)
-
Andrea Bolognani -
Christian Ehrhardt