[libvirt] [PATCH 1/3] tests: Don't crash when creating the config object fails
As observed when building in a chroot and QEMU_USER doesn't exist --- tests/qemuargv2xmltest.c | 3 +++ tests/qemuxml2argvtest.c | 3 +++ 2 files changed, 6 insertions(+) diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 6d7e23e..4cc3749 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -128,6 +128,9 @@ mymain(void) int ret = 0; driver.config = virQEMUDriverConfigNew(false); + if (driver.config == NULL) + return EXIT_FAILURE; + if ((driver.caps = testQemuCapsInit()) == NULL) return EXIT_FAILURE; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 56854dc..13ed4f6 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -501,6 +501,9 @@ mymain(void) } driver.config = virQEMUDriverConfigNew(true); + if (driver.config == NULL) + return EXIT_FAILURE; + VIR_FREE(driver.config->spiceListen); VIR_FREE(driver.config->vncListen); -- 1.9.1
When building packages in a clean chroot the QEMU_USER and QEMU_GROUP don't exist making VirQemuDriverConfigNew fail with privileged=true. Avoid that by not requiring priviliged mode and skipping tests that need it. --- tests/qemuxml2argvtest.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 13ed4f6..f25ef0b 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -484,7 +484,10 @@ mymain(void) { int ret = 0; char *map = NULL; + uid_t user; + gid_t group; bool skipLegacyCPUs = false; + bool privileged = true; abs_top_srcdir = getenv("abs_top_srcdir"); if (!abs_top_srcdir) @@ -500,7 +503,11 @@ mymain(void) return EXIT_FAILURE; } - driver.config = virQEMUDriverConfigNew(true); + if (virGetUserID(QEMU_USER, &user) < 0 || + virGetGroupID(QEMU_GROUP, &group) < 0) + privileged = false; + + driver.config = virQEMUDriverConfigNew(privileged); if (driver.config == NULL) return EXIT_FAILURE; @@ -1160,13 +1167,14 @@ mymain(void) DO_TEST_FAILURE("cpu-host-passthrough", NONE); DO_TEST_FAILURE("cpu-qemu-host-passthrough", QEMU_CAPS_KVM, QEMU_CAPS_CPU_HOST); - - DO_TEST("memtune", QEMU_CAPS_NAME); - DO_TEST("memtune-unlimited", QEMU_CAPS_NAME); - DO_TEST("blkiotune", QEMU_CAPS_NAME); - DO_TEST("blkiotune-device", QEMU_CAPS_NAME); - DO_TEST("cputune", QEMU_CAPS_NAME); - DO_TEST("cputune-zero-shares", QEMU_CAPS_NAME); + if (privileged) { + DO_TEST("memtune", QEMU_CAPS_NAME); + DO_TEST("memtune-unlimited", QEMU_CAPS_NAME); + DO_TEST("blkiotune", QEMU_CAPS_NAME); + DO_TEST("blkiotune-device", QEMU_CAPS_NAME); + DO_TEST("cputune", QEMU_CAPS_NAME); + DO_TEST("cputune-zero-shares", QEMU_CAPS_NAME); + } DO_TEST("numatune-memory", NONE); DO_TEST("numatune-auto-nodeset-invalid", NONE); DO_TEST("numad", NONE); -- 1.9.1
On 04/07/2014 02:02 AM, Guido Günther wrote:
When building packages in a clean chroot the QEMU_USER and QEMU_GROUP don't exist making VirQemuDriverConfigNew fail with privileged=true.
Avoid that by not requiring priviliged mode and skipping tests that need
s/priviliged/privileged/
it. --- tests/qemuxml2argvtest.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
Seems like this is what avoids the fail pointed out in 1/3. It still feels fishy that our testsuite is that dependent on the system (ideally, we'd provide a way to mock things up so that creating the config file NEVER fails when run from the testsuite, even if the uid doesn't exist - because we shouldn't be probing the live system, only our mockups). I'd wait for a second opinion on whether this patch is papering over a bigger problem of depending on the current system state, or whether it is an acceptable way to avoid the issue without investing the effort to tackle at the uid lookup level. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Apr 07, 2014 at 04:40:24PM -0600, Eric Blake wrote:
On 04/07/2014 02:02 AM, Guido Günther wrote:
When building packages in a clean chroot the QEMU_USER and QEMU_GROUP don't exist making VirQemuDriverConfigNew fail with privileged=true.
Avoid that by not requiring priviliged mode and skipping tests that need
s/priviliged/privileged/
it. --- tests/qemuxml2argvtest.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
Seems like this is what avoids the fail pointed out in 1/3. It still feels fishy that our testsuite is that dependent on the system (ideally, we'd provide a way to mock things up so that creating the config file NEVER fails when run from the testsuite, even if the uid doesn't exist - because we shouldn't be probing the live system, only our mockups). I'd wait for a second opinion on whether this patch is papering over a bigger problem of depending on the current system state, or whether it is an acceptable way to avoid the issue without investing the effort to tackle at the uid lookup level.
IMHO we should be passing privileged == false unconditionally, so that we always skip any magic username lookups. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Apr 08, 2014 at 10:09:59AM +0100, Daniel P. Berrange wrote:
On Mon, Apr 07, 2014 at 04:40:24PM -0600, Eric Blake wrote:
On 04/07/2014 02:02 AM, Guido Günther wrote:
When building packages in a clean chroot the QEMU_USER and QEMU_GROUP don't exist making VirQemuDriverConfigNew fail with privileged=true.
Avoid that by not requiring priviliged mode and skipping tests that need
s/priviliged/privileged/
it. --- tests/qemuxml2argvtest.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
Seems like this is what avoids the fail pointed out in 1/3. It still feels fishy that our testsuite is that dependent on the system (ideally, we'd provide a way to mock things up so that creating the config file NEVER fails when run from the testsuite, even if the uid doesn't exist - because we shouldn't be probing the live system, only our mockups). I'd wait for a second opinion on whether this patch is papering over a bigger problem of depending on the current system state, or whether it is an acceptable way to avoid the issue without investing the effort to tackle at the uid lookup level.
IMHO we should be passing privileged == false unconditionally, so that we always skip any magic username lookups.
I would have done that but Martin's 29151830e468f1a9d8006a62702591958a4e3481 did the opposite, so o.k. to revert that? Cheers, -- Guido
On Tue, Apr 08, 2014 at 12:49:25PM +0200, Guido Günther wrote:
On Tue, Apr 08, 2014 at 10:09:59AM +0100, Daniel P. Berrange wrote:
On Mon, Apr 07, 2014 at 04:40:24PM -0600, Eric Blake wrote:
On 04/07/2014 02:02 AM, Guido Günther wrote:
When building packages in a clean chroot the QEMU_USER and QEMU_GROUP don't exist making VirQemuDriverConfigNew fail with privileged=true.
Avoid that by not requiring priviliged mode and skipping tests that need
s/priviliged/privileged/
it. --- tests/qemuxml2argvtest.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
Seems like this is what avoids the fail pointed out in 1/3. It still feels fishy that our testsuite is that dependent on the system (ideally, we'd provide a way to mock things up so that creating the config file NEVER fails when run from the testsuite, even if the uid doesn't exist - because we shouldn't be probing the live system, only our mockups). I'd wait for a second opinion on whether this patch is papering over a bigger problem of depending on the current system state, or whether it is an acceptable way to avoid the issue without investing the effort to tackle at the uid lookup level.
IMHO we should be passing privileged == false unconditionally, so that we always skip any magic username lookups.
I would have done that but Martin's 29151830e468f1a9d8006a62702591958a4e3481 did the opposite, so o.k. to revert that?
Reverting that will make *tune tests fail. We could, however, create our own test config instead of virQEMUDriverConfigNew() or add a parameter to it which will decide on what to do, the least being: diff --git i/src/qemu/qemu_conf.c w/src/qemu/qemu_conf.c index 198ee2f..c8a7f5f 100644 --- i/src/qemu/qemu_conf.c +++ w/src/qemu/qemu_conf.c @@ -120,7 +120,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->privileged = privileged; cfg->uri = privileged ? "qemu:///system" : "qemu:///session"; - if (privileged) { + if (privileged && !test_mode) { if (virGetUserID(QEMU_USER, &cfg->user) < 0) goto error; if (virGetGroupID(QEMU_GROUP, &cfg->group) < 0) -- Martin
On Tue, Apr 08, 2014 at 01:47:14PM +0200, Martin Kletzander wrote:
On Tue, Apr 08, 2014 at 12:49:25PM +0200, Guido Günther wrote:
On Tue, Apr 08, 2014 at 10:09:59AM +0100, Daniel P. Berrange wrote:
On Mon, Apr 07, 2014 at 04:40:24PM -0600, Eric Blake wrote:
On 04/07/2014 02:02 AM, Guido Günther wrote:
When building packages in a clean chroot the QEMU_USER and QEMU_GROUP don't exist making VirQemuDriverConfigNew fail with privileged=true.
Avoid that by not requiring priviliged mode and skipping tests that need
s/priviliged/privileged/
it. --- tests/qemuxml2argvtest.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
Seems like this is what avoids the fail pointed out in 1/3. It still feels fishy that our testsuite is that dependent on the system (ideally, we'd provide a way to mock things up so that creating the config file NEVER fails when run from the testsuite, even if the uid doesn't exist - because we shouldn't be probing the live system, only our mockups). I'd wait for a second opinion on whether this patch is papering over a bigger problem of depending on the current system state, or whether it is an acceptable way to avoid the issue without investing the effort to tackle at the uid lookup level.
IMHO we should be passing privileged == false unconditionally, so that we always skip any magic username lookups.
I would have done that but Martin's 29151830e468f1a9d8006a62702591958a4e3481 did the opposite, so o.k. to revert that?
Reverting that will make *tune tests fail. We could, however, create our own test config instead of virQEMUDriverConfigNew() or add a parameter to it which will decide on what to do, the least being:
What about passing 'false' to ConfigNew() but then manually set 'cfg->privileged = true' on the object we get back. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Apr 08, 2014 at 12:52:36PM +0100, Daniel P. Berrange wrote:
On Tue, Apr 08, 2014 at 01:47:14PM +0200, Martin Kletzander wrote:
On Tue, Apr 08, 2014 at 12:49:25PM +0200, Guido Günther wrote:
On Tue, Apr 08, 2014 at 10:09:59AM +0100, Daniel P. Berrange wrote:
On Mon, Apr 07, 2014 at 04:40:24PM -0600, Eric Blake wrote:
On 04/07/2014 02:02 AM, Guido Günther wrote:
When building packages in a clean chroot the QEMU_USER and QEMU_GROUP don't exist making VirQemuDriverConfigNew fail with privileged=true.
Avoid that by not requiring priviliged mode and skipping tests that need
s/priviliged/privileged/
it. --- tests/qemuxml2argvtest.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
Seems like this is what avoids the fail pointed out in 1/3. It still feels fishy that our testsuite is that dependent on the system (ideally, we'd provide a way to mock things up so that creating the config file NEVER fails when run from the testsuite, even if the uid doesn't exist - because we shouldn't be probing the live system, only our mockups). I'd wait for a second opinion on whether this patch is papering over a bigger problem of depending on the current system state, or whether it is an acceptable way to avoid the issue without investing the effort to tackle at the uid lookup level.
IMHO we should be passing privileged == false unconditionally, so that we always skip any magic username lookups.
I would have done that but Martin's 29151830e468f1a9d8006a62702591958a4e3481 did the opposite, so o.k. to revert that?
Reverting that will make *tune tests fail. We could, however, create our own test config instead of virQEMUDriverConfigNew() or add a parameter to it which will decide on what to do, the least being:
What about passing 'false' to ConfigNew() but then manually set 'cfg->privileged = true' on the object we get back.
That could work. All tests passed on my setup like that. And it doesn't seem weird since we're playing with the config a lot in the tests. Martin
On Tue, Apr 08, 2014 at 03:03:55PM +0200, Martin Kletzander wrote:
On Tue, Apr 08, 2014 at 12:52:36PM +0100, Daniel P. Berrange wrote:
On Tue, Apr 08, 2014 at 01:47:14PM +0200, Martin Kletzander wrote:
On Tue, Apr 08, 2014 at 12:49:25PM +0200, Guido Günther wrote:
On Tue, Apr 08, 2014 at 10:09:59AM +0100, Daniel P. Berrange wrote:
On Mon, Apr 07, 2014 at 04:40:24PM -0600, Eric Blake wrote:
On 04/07/2014 02:02 AM, Guido Günther wrote: > When building packages in a clean chroot the QEMU_USER and QEMU_GROUP > don't exist making VirQemuDriverConfigNew fail with privileged=true. > > Avoid that by not requiring priviliged mode and skipping tests that need
s/priviliged/privileged/
> it. > --- > tests/qemuxml2argvtest.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-)
Seems like this is what avoids the fail pointed out in 1/3. It still feels fishy that our testsuite is that dependent on the system (ideally, we'd provide a way to mock things up so that creating the config file NEVER fails when run from the testsuite, even if the uid doesn't exist - because we shouldn't be probing the live system, only our mockups). I'd wait for a second opinion on whether this patch is papering over a bigger problem of depending on the current system state, or whether it is an acceptable way to avoid the issue without investing the effort to tackle at the uid lookup level.
IMHO we should be passing privileged == false unconditionally, so that we always skip any magic username lookups.
I would have done that but Martin's 29151830e468f1a9d8006a62702591958a4e3481 did the opposite, so o.k. to revert that?
Reverting that will make *tune tests fail. We could, however, create our own test config instead of virQEMUDriverConfigNew() or add a parameter to it which will decide on what to do, the least being:
What about passing 'false' to ConfigNew() but then manually set 'cfg->privileged = true' on the object we get back.
That could work. All tests passed on my setup like that. And it doesn't seem weird since we're playing with the config a lot in the tests.
I do wonder if my approach wouldn't be cleaner since it doesn't poke into the objects internals. Cheers, -- Guido
On 04/08/2014 07:06 AM, Guido Günther wrote:
What about passing 'false' to ConfigNew() but then manually set 'cfg->privileged = true' on the object we get back.
That could work. All tests passed on my setup like that. And it doesn't seem weird since we're playing with the config a lot in the tests.
I do wonder if my approach wouldn't be cleaner since it doesn't poke into the objects internals.
Your approach skips the test, which means less testsuite coverage. I'd rather poke into internals to make the testsuite independent of the environment while still maximizing coverage. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Apr 08, 2014 at 07:27:06AM -0600, Eric Blake wrote:
On 04/08/2014 07:06 AM, Guido Günther wrote:
What about passing 'false' to ConfigNew() but then manually set 'cfg->privileged = true' on the object we get back.
That could work. All tests passed on my setup like that. And it doesn't seem weird since we're playing with the config a lot in the tests.
I do wonder if my approach wouldn't be cleaner since it doesn't poke into the objects internals.
Your approach skips the test, which means less testsuite coverage. I'd rather poke into internals to make the testsuite independent of the environment while still maximizing coverage.
O.k. then, ACK. -- Guido
When building packages in a clean chroot the QEMU_USER and QEMU_GROUP don't exist making VirQemuDriverConfigNew fail with privileged=true. Avoid that by not requiring priviliged mode upfront but setting it later so we skip the user/group existence check. This solution was suggested by Daniel P. Berrange and tested by Martin Kletzander. --- tests/qemuxml2argvtest.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a1ef2b8..14482fd 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -499,9 +499,11 @@ mymain(void) return EXIT_FAILURE; } - driver.config = virQEMUDriverConfigNew(true); + driver.config = virQEMUDriverConfigNew(false); if (driver.config == NULL) return EXIT_FAILURE; + else + driver.config->privileged = true; VIR_FREE(driver.config->spiceListen); VIR_FREE(driver.config->vncListen); -- 1.9.2
On 05/03/2014 06:16 AM, Guido Günther wrote:
When building packages in a clean chroot the QEMU_USER and QEMU_GROUP don't exist making VirQemuDriverConfigNew fail with privileged=true.
Avoid that by not requiring priviliged mode upfront but setting it later
s/priviliged/privileged/
so we skip the user/group existence check.
This solution was suggested by Daniel P. Berrange and tested by Martin Kletzander. ---
tests/qemuxml2argvtest.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
ACK; safe for 1.2.4 -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Sat, May 03, 2014 at 07:39:40AM -0600, Eric Blake wrote:
On 05/03/2014 06:16 AM, Guido Günther wrote:
When building packages in a clean chroot the QEMU_USER and QEMU_GROUP don't exist making VirQemuDriverConfigNew fail with privileged=true.
Avoid that by not requiring priviliged mode upfront but setting it later
s/priviliged/privileged/
so we skip the user/group existence check.
This solution was suggested by Daniel P. Berrange and tested by Martin Kletzander. ---
tests/qemuxml2argvtest.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
ACK; safe for 1.2.4
Pushed with the above fixed. Thanks, -- Guido
to avoid CCLD storagevolxml2argvtest /usr/bin/ld: ../src/.libs/libvirt_driver_storage_impl.a(libvirt_driver_storage_impl_la-storage_backend.o): undefined reference to symbol 'xmlFreeDoc@@LIBXML2_2.4.30' //usr/lib/x86_64-linux-gnu/libxml2.so.2: error adding symbols: DSO missing from command line --- tests/Makefile.am | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index 6e15af8..8e011a8 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -681,6 +681,7 @@ storagevolxml2argvtest_SOURCES = \ storagevolxml2argvtest.c \ testutils.c testutils.h storagevolxml2argvtest_LDADD = \ + $(LIBXML_LIBS) \ ../src/libvirt_driver_storage_impl.la $(LDADDS) else ! WITH_STORAGE -- 1.9.1
On 04/07/2014 02:02 AM, Guido Günther wrote:
to avoid
CCLD storagevolxml2argvtest /usr/bin/ld: ../src/.libs/libvirt_driver_storage_impl.a(libvirt_driver_storage_impl_la-storage_backend.o): undefined reference to symbol 'xmlFreeDoc@@LIBXML2_2.4.30' //usr/lib/x86_64-linux-gnu/libxml2.so.2: error adding symbols: DSO missing from command line --- tests/Makefile.am | 1 + 1 file changed, 1 insertion(+)
ACK.
diff --git a/tests/Makefile.am b/tests/Makefile.am index 6e15af8..8e011a8 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -681,6 +681,7 @@ storagevolxml2argvtest_SOURCES = \ storagevolxml2argvtest.c \ testutils.c testutils.h storagevolxml2argvtest_LDADD = \ + $(LIBXML_LIBS) \ ../src/libvirt_driver_storage_impl.la $(LDADDS)
else ! WITH_STORAGE
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/07/2014 02:02 AM, Guido Günther wrote:
As observed when building in a chroot and QEMU_USER doesn't exist --- tests/qemuargv2xmltest.c | 3 +++ tests/qemuxml2argvtest.c | 3 +++ 2 files changed, 6 insertions(+)
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 6d7e23e..4cc3749 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -128,6 +128,9 @@ mymain(void) int ret = 0;
driver.config = virQEMUDriverConfigNew(false); + if (driver.config == NULL) + return EXIT_FAILURE; +
Still a failure, but at least better than a crash. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Apr 07, 2014 at 04:34:57PM -0600, Eric Blake wrote:
On 04/07/2014 02:02 AM, Guido Günther wrote:
As observed when building in a chroot and QEMU_USER doesn't exist --- tests/qemuargv2xmltest.c | 3 +++ tests/qemuxml2argvtest.c | 3 +++ 2 files changed, 6 insertions(+)
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 6d7e23e..4cc3749 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -128,6 +128,9 @@ mymain(void) int ret = 0;
driver.config = virQEMUDriverConfigNew(false); + if (driver.config == NULL) + return EXIT_FAILURE; +
Still a failure, but at least better than a crash. Pushed. Thanks, -- Guido
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Guido Günther -
Martin Kletzander