[libvirt] [PATCH 0/6] qemu: Add support for 'write-cache' disk frontend argument (blockdev-add saga)
'write-cache' is one of the booleans the 'cache' attribute of -drive translates to. Since we are getting rid of -drive this needs to be implemented in a different way. Peter Krempa (6): tests: utils: Allow parsing test capability file without virCaps tests: qemuxm2argv: Add infrastructure for testing with real qemuCaps qemu: domain: Add helper for translating disk cachemode to qemu flags qemu: caps: Add capability for 'write-cache' parameter of disk frontends qemu: Format 'write-cache' parameter for disk frontends tests: qemuxml2argv: Test formatting of 'write-cache' parameter src/qemu/qemu_capabilities.c | 5 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 27 ++++++++ src/qemu/qemu_domain.c | 75 ++++++++++++++++++++++ src/qemu/qemu_domain.h | 6 ++ tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + tests/qemuxml2argvdata/disk-drive-write-cache.args | 45 +++++++++++++ tests/qemuxml2argvdata/disk-drive-write-cache.xml | 45 +++++++++++++ tests/qemuxml2argvtest.c | 32 +++++++++ tests/testutilsqemu.c | 21 ++++-- tests/testutilsqemu.h | 3 + 26 files changed, 270 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.args create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.xml -- 2.16.2
virCaps was used only to propagate the host architecture, so the function can be extracted in a way which does not require it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemu.c | 21 +++++++++++++++------ tests/testutilsqemu.h | 3 +++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index f8182033fc..9671a46f12 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -486,16 +486,13 @@ qemuTestSetHostCPU(virCapsPtr caps, virQEMUCapsPtr -qemuTestParseCapabilities(virCapsPtr caps, - const char *capsFile) +qemuTestParseCapabilitiesArch(virArch arch, + const char *capsFile) { virQEMUCapsPtr qemuCaps = NULL; - if (!caps) - return NULL; - if (!(qemuCaps = virQEMUCapsNew()) || - virQEMUCapsLoadCache(caps->host.arch, qemuCaps, capsFile) < 0) + virQEMUCapsLoadCache(arch, qemuCaps, capsFile) < 0) goto error; return qemuCaps; @@ -505,6 +502,18 @@ qemuTestParseCapabilities(virCapsPtr caps, return NULL; } + +virQEMUCapsPtr +qemuTestParseCapabilities(virCapsPtr caps, + const char *capsFile) +{ + if (!caps) + return NULL; + + return qemuTestParseCapabilitiesArch(caps->host.arch, capsFile); +} + + void qemuTestDriverFree(virQEMUDriver *driver) { virMutexDestroy(&driver->lock); diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index f29c6e5d62..7ae8324933 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -16,6 +16,9 @@ enum { virCapsPtr testQemuCapsInit(void); virDomainXMLOptionPtr testQemuXMLConfInit(void); + +virQEMUCapsPtr qemuTestParseCapabilitiesArch(virArch arch, + const char *capsFile); virQEMUCapsPtr qemuTestParseCapabilities(virCapsPtr caps, const char *capsFile); -- 2.16.2
On 04/04/2018 04:13 AM, Peter Krempa wrote:
virCaps was used only to propagate the host architecture, so the function can be extracted in a way which does not require it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemu.c | 21 +++++++++++++++------ tests/testutilsqemu.h | 3 +++ 2 files changed, 18 insertions(+), 6 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Wed, Apr 04, 2018 at 10:13:54AM +0200, Peter Krempa wrote:
virCaps was used only to propagate the host architecture, so the function can be extracted in a way which does not require it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemu.c | 21 +++++++++++++++------ tests/testutilsqemu.h | 3 +++ 2 files changed, 18 insertions(+), 6 deletions(-)
ACK Jano
Allow testing of XML->argv conversion with using a real capability map as used in the qemucapabilitiestest. This allows specifying the required qemu version with the test rather than having to enumerate all the required capabilities. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d79913dd0a..c540ce2f50 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -654,6 +654,37 @@ mymain(void) if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0) return EXIT_FAILURE; + +# define DO_TEST_CAPS_ARCH(name, migrateFrom, flags, \ + parseFlags, gic, arch, ver) \ + do { \ + static struct testInfo info = { \ + name, NULL, migrateFrom, migrateFrom ? 7 : -1, (flags), parseFlags, \ + false, NULL \ + }; \ + info.skipLegacyCPUs = skipLegacyCPUs; \ + if (testInitQEMUCaps(&info, gic) < 0) \ + return EXIT_FAILURE; \ + if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ + abs_srcdir "/qemucapabilitiesdata/caps_" \ + ver "." arch ".xml"))) \ + return EXIT_FAILURE; \ + if (virTestRun("QEMU XML-2-ARGV " name, \ + testCompareXMLToArgv, &info) < 0) \ + ret = -1; \ + if (info.vm && virTestRun("QEMU XML-2-startup-XML " name, \ + testCompareXMLToStartupXML, &info) < 0) \ + ret = -1; \ + virObjectUnref(info.qemuCaps); \ + virObjectUnref(info.vm); \ + } while (0) + +# define DO_TEST_CAPS_FULL(name, flags, parseFlags, ver) \ + DO_TEST_CAPS_ARCH(name, NULL, flags, parseFlags, GIC_NONE, "x86_64", ver) + +# define DO_TEST_CAPS(name, ver) \ + DO_TEST_CAPS_FULL(name, 0, 0, ver) + # define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \ parseFlags, gic, ...) \ do { \ -- 2.16.2
On 04/04/2018 04:13 AM, Peter Krempa wrote:
Allow testing of XML->argv conversion with using a real capability map as used in the qemucapabilitiestest. This allows specifying the required qemu version with the test rather than having to enumerate all the required capabilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d79913dd0a..c540ce2f50 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -654,6 +654,37 @@ mymain(void) if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0) return EXIT_FAILURE;
+ +# define DO_TEST_CAPS_ARCH(name, migrateFrom, flags, \ + parseFlags, gic, arch, ver) \ + do { \ + static struct testInfo info = { \ + name, NULL, migrateFrom, migrateFrom ? 7 : -1, (flags), parseFlags, \ + false, NULL \ + }; \ + info.skipLegacyCPUs = skipLegacyCPUs; \ + if (testInitQEMUCaps(&info, gic) < 0) \ + return EXIT_FAILURE; \ + if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ + abs_srcdir "/qemucapabilitiesdata/caps_" \ + ver "." arch ".xml"))) \ + return EXIT_FAILURE; \ + if (virTestRun("QEMU XML-2-ARGV " name, \ + testCompareXMLToArgv, &info) < 0) \ + ret = -1; \ + if (info.vm && virTestRun("QEMU XML-2-startup-XML " name, \ + testCompareXMLToStartupXML, &info) < 0) \ + ret = -1; \ + virObjectUnref(info.qemuCaps); \ + virObjectUnref(info.vm); \ + } while (0) + +# define DO_TEST_CAPS_FULL(name, flags, parseFlags, ver) \ + DO_TEST_CAPS_ARCH(name, NULL, flags, parseFlags, GIC_NONE, "x86_64", ver)
Assumes x64_64...
+ +# define DO_TEST_CAPS(name, ver) \ + DO_TEST_CAPS_FULL(name, 0, 0, ver) + # define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \ parseFlags, gic, ...) \ do { \
Do you expect to see wide spread and future use of this model as opposed to the existing DO_TEST model? Shouldn't at least some of the existing tests be converted, then? Perhaps the macro(s) should be DO_TEST_VERS[ION] or DO_TEST_<ARCH>_VERS instead since DO_TEST runs the test with specific capabilities and this new model runs the test with a specific version for a specific platform. How do or should we enforce that when adding a new test from this point forward that the DO_TEST_CAPS model is chosen over DO_TEST? I suppose there is still value in DO_TEST since one can pick and choose which CAP is needed - although I'll freely admit that's usually a cut-n-paste from some other test and change the name type activity. I think I have too many questions for an R-b at this point. John
On Wed, Apr 11, 2018 at 11:16:27AM -0400, John Ferlan wrote:
On 04/04/2018 04:13 AM, Peter Krempa wrote:
Allow testing of XML->argv conversion with using a real capability map as used in the qemucapabilitiestest. This allows specifying the required qemu version with the test rather than having to enumerate all the required capabilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d79913dd0a..c540ce2f50 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -654,6 +654,37 @@ mymain(void) if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0) return EXIT_FAILURE;
+ +# define DO_TEST_CAPS_ARCH(name, migrateFrom, flags, \ + parseFlags, gic, arch, ver) \ + do { \ + static struct testInfo info = { \ + name, NULL, migrateFrom, migrateFrom ? 7 : -1, (flags), parseFlags, \ + false, NULL \ + }; \ + info.skipLegacyCPUs = skipLegacyCPUs; \ + if (testInitQEMUCaps(&info, gic) < 0) \ + return EXIT_FAILURE; \ + if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ + abs_srcdir "/qemucapabilitiesdata/caps_" \ + ver "." arch ".xml"))) \ + return EXIT_FAILURE; \ + if (virTestRun("QEMU XML-2-ARGV " name, \ + testCompareXMLToArgv, &info) < 0) \ + ret = -1; \ + if (info.vm && virTestRun("QEMU XML-2-startup-XML " name, \ + testCompareXMLToStartupXML, &info) < 0) \ + ret = -1; \ + virObjectUnref(info.qemuCaps); \ + virObjectUnref(info.vm); \ + } while (0) + +# define DO_TEST_CAPS_FULL(name, flags, parseFlags, ver) \ + DO_TEST_CAPS_ARCH(name, NULL, flags, parseFlags, GIC_NONE, "x86_64", ver)
Assumes x64_64...
+ +# define DO_TEST_CAPS(name, ver) \ + DO_TEST_CAPS_FULL(name, 0, 0, ver) + # define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \ parseFlags, gic, ...) \ do { \
Do you expect to see wide spread and future use of this model as opposed to the existing DO_TEST model? Shouldn't at least some of the existing tests be converted, then?
Yes to both.
Perhaps the macro(s) should be DO_TEST_VERS[ION] or DO_TEST_<ARCH>_VERS instead since DO_TEST runs the test with specific capabilities and this new model runs the test with a specific version for a specific platform.
How do or should we enforce that when adding a new test from this point forward that the DO_TEST_CAPS model is chosen over DO_TEST? I suppose there is still value in DO_TEST since one can pick and choose which CAP is needed - although I'll freely admit that's usually a cut-n-paste from some other test and change the name type activity.
The problem with that approach is that we're testing against capability combinations that might never occur in the wild. I actually remember fixing some bug that was supposed to be covered by a unit test, but it was missing the important capability. Enforcement by review ought to be enough for everybody. Though there still might be features that you can compile out even from QEMU versions that otherwise have the capability, but we don't add negative tests for that often. Jano
I think I have too many questions for an R-b at this point.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Apr 15, 2018 at 15:29:23 +0200, Ján Tomko wrote:
On Wed, Apr 11, 2018 at 11:16:27AM -0400, John Ferlan wrote:
On 04/04/2018 04:13 AM, Peter Krempa wrote:
Allow testing of XML->argv conversion with using a real capability map as used in the qemucapabilitiestest. This allows specifying the required qemu version with the test rather than having to enumerate all the required capabilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
Perhaps the macro(s) should be DO_TEST_VERS[ION] or DO_TEST_<ARCH>_VERS instead since DO_TEST runs the test with specific capabilities and this new model runs the test with a specific version for a specific platform.
How do or should we enforce that when adding a new test from this point forward that the DO_TEST_CAPS model is chosen over DO_TEST? I suppose there is still value in DO_TEST since one can pick and choose which CAP is needed - although I'll freely admit that's usually a cut-n-paste from some other test and change the name type activity.
The problem with that approach is that we're testing against capability combinations that might never occur in the wild.
I actually remember fixing some bug that was supposed to be covered by a unit test, but it was missing the important capability.
Actually a very good example is the recent series of adding the 'discard-data' attribute to memory devices. Since the new feature is decided with capability bits, all existing tests were still passing, but the attribute would be added to NVDIMM memory devices which would delete the contents. I think we also need to add a way to take the latest caps present in the repo to be used with the bulk of tests rather than binding it to a specific version. That way we can see whether the command line is stable when we add a capability based test. If the command line will change, the test case then can be split for the specific version of qemu capabilities, so that the contemporary test case still captures the new case, but the old behaviour is kept too.
On Wed, Apr 11, 2018 at 11:16:27 -0400, John Ferlan wrote:
On 04/04/2018 04:13 AM, Peter Krempa wrote:
Allow testing of XML->argv conversion with using a real capability map as used in the qemucapabilitiestest. This allows specifying the required qemu version with the test rather than having to enumerate all the required capabilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d79913dd0a..c540ce2f50 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -654,6 +654,37 @@ mymain(void) if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0) return EXIT_FAILURE;
+ +# define DO_TEST_CAPS_ARCH(name, migrateFrom, flags, \ + parseFlags, gic, arch, ver) \ + do { \ + static struct testInfo info = { \ + name, NULL, migrateFrom, migrateFrom ? 7 : -1, (flags), parseFlags, \ + false, NULL \ + }; \ + info.skipLegacyCPUs = skipLegacyCPUs; \ + if (testInitQEMUCaps(&info, gic) < 0) \ + return EXIT_FAILURE; \ + if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ + abs_srcdir "/qemucapabilitiesdata/caps_" \ + ver "." arch ".xml"))) \ + return EXIT_FAILURE; \ + if (virTestRun("QEMU XML-2-ARGV " name, \ + testCompareXMLToArgv, &info) < 0) \ + ret = -1; \ + if (info.vm && virTestRun("QEMU XML-2-startup-XML " name, \ + testCompareXMLToStartupXML, &info) < 0) \ + ret = -1; \ + virObjectUnref(info.qemuCaps); \ + virObjectUnref(info.vm); \ + } while (0) + +# define DO_TEST_CAPS_FULL(name, flags, parseFlags, ver) \ + DO_TEST_CAPS_ARCH(name, NULL, flags, parseFlags, GIC_NONE, "x86_64", ver)
Assumes x64_64...
Yes. Intentionally. Hipster-arches can introduce their own helpers.
+ +# define DO_TEST_CAPS(name, ver) \ + DO_TEST_CAPS_FULL(name, 0, 0, ver) + # define DO_TEST_FULL(name, migrateFrom, migrateFd, flags, \ parseFlags, gic, ...) \ do { \
Do you expect to see wide spread and future use of this model as opposed to the existing DO_TEST model? Shouldn't at least some of the existing tests be converted, then?
I hope so, actually for both cases.
Perhaps the macro(s) should be DO_TEST_VERS[ION] or DO_TEST_<ARCH>_VERS instead since DO_TEST runs the test with specific capabilities and this new model runs the test with a specific version for a specific platform.
Hmm, yes. I can rename it to DO_TEST_VERSION. The default will be for x86. As said, other arches can add their own macros.
How do or should we enforce that when adding a new test from this point forward that the DO_TEST_CAPS model is chosen over DO_TEST? I suppose
I think reviewers should ask for using the new test approach for any positive cases, so that we don't test something that is not possible in the wild. In cases when it's required to do so (negative tests, or weird config tests) use of DO_TEST is justified.
there is still value in DO_TEST since one can pick and choose which CAP is needed - although I'll freely admit that's usually a cut-n-paste from some other test and change the name type activity.
I think I have too many questions for an R-b at this point.
John
s/xm/xml/ in the summary On Wed, Apr 04, 2018 at 10:13:55AM +0200, Peter Krempa wrote:
Allow testing of XML->argv conversion with using a real capability map as used in the qemucapabilitiestest. This allows specifying the required qemu version with the test rather than having to enumerate all the required capabilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
ACK Jano
Add helper which will map values of disk cache mode to the flags which are accepted by various parts of the qemu block layer. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 6 ++++ 2 files changed, 81 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9d1c33b54a..20333c9568 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11920,6 +11920,81 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk, } +/** + * qemuDomainDiskCachemodeFlags: + * + * Converts disk cachemode to the cache mode options for qemu. Returns -1 for + * invalid @cachemode values and fills the flags and returns 0 on success. + * Flags may be NULL. + */ +int +qemuDomainDiskCachemodeFlags(int cachemode, + bool *writeback, + bool *direct, + bool *noflush) +{ + bool dummy; + + if (!writeback) + writeback = &dummy; + + if (!direct) + direct = &dummy; + + if (!noflush) + noflush = &dummy; + + /* Mapping of cache modes to the attributes according to qemu-options.hx + * │ cache.writeback cache.direct cache.no-flush + * ─────────────┼───────────────────────────────────────────────── + * writeback │ true false false + * none │ true true false + * writethrough │ false false false + * directsync │ false true false + * unsafe │ true false true + */ + switch ((virDomainDiskCache) cachemode) { + case VIR_DOMAIN_DISK_CACHE_DISABLE: /* 'none' */ + *writeback = true; + *direct = true; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_WRITETHRU: + *writeback = false; + *direct = false; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_WRITEBACK: + *writeback = true; + *direct = false; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_DIRECTSYNC: + *writeback = false; + *direct = true; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_UNSAFE: + *writeback = true; + *direct = false; + *noflush = true; + break; + + case VIR_DOMAIN_DISK_CACHE_DEFAULT: + case VIR_DOMAIN_DISK_CACHE_LAST: + default: + virReportEnumRangeError(virDomainDiskCache, cachemode); + return -1; + } + + return 0; +} + + void qemuProcessEventFree(struct qemuProcessEvent *event) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 21e12f6594..bbc2e83760 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1003,4 +1003,10 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk, qemuDomainObjPrivatePtr priv, virQEMUDriverConfigPtr cfg); +int +qemuDomainDiskCachemodeFlags(int cachemode, + bool *writeback, + bool *direct, + bool *noflush); + #endif /* __QEMU_DOMAIN_H__ */ -- 2.16.2
On 04/04/2018 04:13 AM, Peter Krempa wrote:
Add helper which will map values of disk cache mode to the flags which are accepted by various parts of the qemu block layer.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 6 ++++ 2 files changed, 81 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9d1c33b54a..20333c9568 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11920,6 +11920,81 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk, }
+/** + * qemuDomainDiskCachemodeFlags: + * + * Converts disk cachemode to the cache mode options for qemu. Returns -1 for + * invalid @cachemode values and fills the flags and returns 0 on success. + * Flags may be NULL. + */ +int +qemuDomainDiskCachemodeFlags(int cachemode, + bool *writeback, + bool *direct, + bool *noflush)
Yuck, multiple boolean returns.... and when a new mode is added, we possibly get another bool argument leading to another useless argument. This could conceivably do nothing if each of the return args was NULL. It seems what you're doing is translating or converting the VIR_DOMAIN_DISK_CACHE_* values into some mask that you'll use elsewhere perhaps similar to virDomainDefFormatConvertXMLFlags. I think this just returns an unsigned int mask that's particular to some to be defined enum that would set the right bits so that when building the command line we can add features based on those bits. That way it really doesn't matter that direct and noflush aren't used (yet) in any patch that's been posted...
+{ + bool dummy; + + if (!writeback) + writeback = &dummy; + + if (!direct) + direct = &dummy; + + if (!noflush) + noflush = &dummy; + + /* Mapping of cache modes to the attributes according to qemu-options.hx + * │ cache.writeback cache.direct cache.no-flush + * ─────────────┼───────────────────────────────────────────────── + * writeback │ true false false + * none │ true true false + * writethrough │ false false false + * directsync │ false true false + * unsafe │ true false true + */ + switch ((virDomainDiskCache) cachemode) { + case VIR_DOMAIN_DISK_CACHE_DISABLE: /* 'none' */ + *writeback = true; + *direct = true; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_WRITETHRU: + *writeback = false; + *direct = false; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_WRITEBACK: + *writeback = true; + *direct = false; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_DIRECTSYNC: + *writeback = false; + *direct = true; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_UNSAFE: + *writeback = true; + *direct = false; + *noflush = true; + break; + + case VIR_DOMAIN_DISK_CACHE_DEFAULT:
Based on the patch 5 consumer qemuBuildDriveDevCacheStr - this would DEFAULT would never happen.
+ case VIR_DOMAIN_DISK_CACHE_LAST: + default:
Considering how/when this helper is being used, one would hope by the time this is used that cachemode has already been verified to be in range. It would seem that's true because virDomainDiskDefDriverParseXML would have failed the virDomainDiskCacheTypeFromString. John
+ virReportEnumRangeError(virDomainDiskCache, cachemode); + return -1; + } + + return 0; +} + + void qemuProcessEventFree(struct qemuProcessEvent *event) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 21e12f6594..bbc2e83760 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1003,4 +1003,10 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk, qemuDomainObjPrivatePtr priv, virQEMUDriverConfigPtr cfg);
+int +qemuDomainDiskCachemodeFlags(int cachemode, + bool *writeback, + bool *direct, + bool *noflush); + #endif /* __QEMU_DOMAIN_H__ */
On Wed, Apr 11, 2018 at 11:17:46 -0400, John Ferlan wrote:
On 04/04/2018 04:13 AM, Peter Krempa wrote:
Add helper which will map values of disk cache mode to the flags which are accepted by various parts of the qemu block layer.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 6 ++++ 2 files changed, 81 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9d1c33b54a..20333c9568 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11920,6 +11920,81 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk, }
+/** + * qemuDomainDiskCachemodeFlags: + * + * Converts disk cachemode to the cache mode options for qemu. Returns -1 for + * invalid @cachemode values and fills the flags and returns 0 on success. + * Flags may be NULL. + */ +int +qemuDomainDiskCachemodeFlags(int cachemode, + bool *writeback, + bool *direct, + bool *noflush)
Yuck, multiple boolean returns.... and when a new mode is added, we possibly get another bool argument leading to another useless argument. This could conceivably do nothing if each of the return args was NULL.
It seems what you're doing is translating or converting the VIR_DOMAIN_DISK_CACHE_* values into some mask that you'll use elsewhere perhaps similar to virDomainDefFormatConvertXMLFlags.
Exactly. Even the function comment says that.
I think this just returns an unsigned int mask that's particular to some to be defined enum that would set the right bits so that when building the command line we can add features based on those bits.
It can't return unsigned since due to the new switch/case handling for enums we need to be able return failure. Also re-compressing this to a bitmap is obviously possible, but rather pointless, since any user will need to re-extract the presence of individual bits.
That way it really doesn't matter that direct and noflush aren't used (yet) in any patch that's been posted...
I fail to see how that's different from re-extracting them from a bitmap.
+{ + bool dummy; + + if (!writeback) + writeback = &dummy; + + if (!direct) + direct = &dummy; + + if (!noflush) + noflush = &dummy; + + /* Mapping of cache modes to the attributes according to qemu-options.hx + * │ cache.writeback cache.direct cache.no-flush + * ─────────────┼───────────────────────────────────────────────── + * writeback │ true false false + * none │ true true false + * writethrough │ false false false + * directsync │ false true false + * unsafe │ true false true + */ + switch ((virDomainDiskCache) cachemode) { + case VIR_DOMAIN_DISK_CACHE_DISABLE: /* 'none' */ + *writeback = true; + *direct = true; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_WRITETHRU: + *writeback = false; + *direct = false; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_WRITEBACK: + *writeback = true; + *direct = false; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_DIRECTSYNC: + *writeback = false; + *direct = true; + *noflush = false; + break; + + case VIR_DOMAIN_DISK_CACHE_UNSAFE: + *writeback = true; + *direct = false; + *noflush = true; + break; + + case VIR_DOMAIN_DISK_CACHE_DEFAULT:
Based on the patch 5 consumer qemuBuildDriveDevCacheStr - this would DEFAULT would never happen.
+ case VIR_DOMAIN_DISK_CACHE_LAST: + default:
Considering how/when this helper is being used, one would hope by the time this is used that cachemode has already been verified to be in range. It would seem that's true because virDomainDiskDefDriverParseXML would have failed the virDomainDiskCacheTypeFromString.
No, callers only verify one end of the range. The other end of the range is not verified. Also we recently decided to always include the default case even when we validate the callers or fully enumerate all values, since it does not prevent assigning any invalid value.
On Wed, Apr 04, 2018 at 10:13:56AM +0200, Peter Krempa wrote:
Add helper which will map values of disk cache mode to the flags which are accepted by various parts of the qemu block layer.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 6 ++++ 2 files changed, 81 insertions(+)
ACK Jano
QEMU translates the cache mode of a disk internally into 3 flags. 'write-cache' is a flag of the frontend while others are flag of the backing storage. Add capability which will allow expressing it via the frontend attribute. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 18 files changed, 22 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 959c27f3bf..dea2191e03 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -467,6 +467,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-mouse-ccw", "virtio-tablet-ccw", "qcow2-luks", + "disk-write-cache", ); @@ -1727,6 +1728,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { { "disable-legacy", QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY }, { "iommu_platform", QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM }, { "ats", QEMU_CAPS_VIRTIO_PCI_ATS }, + { "write-cache", QEMU_CAPS_DISK_WRITE_CACHE }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioNet[] = { @@ -1764,11 +1766,13 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsSCSIDisk[] = { { "channel", QEMU_CAPS_SCSI_DISK_CHANNEL }, { "wwn", QEMU_CAPS_SCSI_DISK_WWN }, { "share-rw", QEMU_CAPS_DISK_SHARE_RW }, + { "write-cache", QEMU_CAPS_DISK_WRITE_CACHE }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsIDEDrive[] = { { "wwn", QEMU_CAPS_IDE_DRIVE_WWN }, { "share-rw", QEMU_CAPS_DISK_SHARE_RW }, + { "write-cache", QEMU_CAPS_DISK_WRITE_CACHE }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsPiix4PM[] = { @@ -1800,6 +1804,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQ35PCIHost[] = { static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsUSBStorage[] = { { "removable", QEMU_CAPS_USB_STORAGE_REMOVABLE }, { "share-rw", QEMU_CAPS_DISK_SHARE_RW }, + { "write-cache", QEMU_CAPS_DISK_WRITE_CACHE }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsKVMPit[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 2203c28aa0..b2a48304f4 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -451,6 +451,7 @@ typedef enum { QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW, /* -device virtio-mouse-ccw */ QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */ QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */ + QEMU_CAPS_DISK_WRITE_CACHE, /* qemu block frontends support write-cache param */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml index 7585e02da2..2cdb246563 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml @@ -187,6 +187,7 @@ <flag name='pl011'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> + <flag name='disk-write-cache'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>303541</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml index 153e199c41..26c39051f7 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml @@ -186,6 +186,7 @@ <flag name='machine.pseries.max-cpu-compat'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> + <flag name='disk-write-cache'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>382824</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml index c4be3fca51..ce7c6eff09 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml @@ -147,6 +147,7 @@ <flag name='iscsi.password-secret'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> + <flag name='disk-write-cache'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>303326</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml index 4dd5602014..4e6dac34fb 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml @@ -230,6 +230,7 @@ <flag name='isa-serial'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> + <flag name='disk-write-cache'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>344938</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml index cbd645ae93..3cdea03a71 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -151,6 +151,7 @@ <flag name='virtio-mouse-ccw'/> <flag name='virtio-tablet-ccw'/> <flag name='qcow2-luks'/> + <flag name='disk-write-cache'/> <version>2011000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>342058</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml index ec2eec17f4..81d67388f4 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -188,6 +188,7 @@ <flag name='pl011'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> + <flag name='disk-write-cache'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>342346</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index 1122d6408b..b8b62c85ff 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -186,6 +186,7 @@ <flag name='machine.pseries.max-cpu-compat'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> + <flag name='disk-write-cache'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>419215</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 191b1e0e37..c634800340 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -151,6 +151,7 @@ <flag name='virtio-mouse-ccw'/> <flag name='virtio-tablet-ccw'/> <flag name='qcow2-luks'/> + <flag name='disk-write-cache'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index aa5de811e1..97d8355451 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -226,6 +226,7 @@ <flag name='isa-serial'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> + <flag name='disk-write-cache'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>390060</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml index b58c3a1bda..7547d66479 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml @@ -137,6 +137,7 @@ <flag name='virtio-blk.num-queues'/> <flag name='sclplmconsole'/> <flag name='dump-completed'/> + <flag name='disk-write-cache'/> <version>2007000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>216732</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml index 736e61aeb2..e4d53f3dd1 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml @@ -210,6 +210,7 @@ <flag name='virtio-blk.num-queues'/> <flag name='isa-serial'/> <flag name='dump-completed'/> + <flag name='disk-write-cache'/> <version>2007000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>239029</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml index 2b91337fde..ee7d4d6b81 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml @@ -139,6 +139,7 @@ <flag name='virtio-blk.num-queues'/> <flag name='sclplmconsole'/> <flag name='dump-completed'/> + <flag name='disk-write-cache'/> <version>2007093</version> <kvmVersion>0</kvmVersion> <microcodeVersion>241633</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml index df6f7be340..f815c5c324 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml @@ -212,6 +212,7 @@ <flag name='virtio-blk.num-queues'/> <flag name='isa-serial'/> <flag name='dump-completed'/> + <flag name='disk-write-cache'/> <version>2008000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>255684</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml index c83b5211f3..1c4082ac52 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml @@ -178,6 +178,7 @@ <flag name='iscsi.password-secret'/> <flag name='isa-serial'/> <flag name='dump-completed'/> + <flag name='disk-write-cache'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>346538</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml index ac8d145d19..67ee72155b 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml @@ -142,6 +142,7 @@ <flag name='disk-share-rw'/> <flag name='iscsi.password-secret'/> <flag name='dump-completed'/> + <flag name='disk-write-cache'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>265051</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml index 13fc8c143f..97908bf5a0 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml @@ -225,6 +225,7 @@ <flag name='iscsi.password-secret'/> <flag name='isa-serial'/> <flag name='dump-completed'/> + <flag name='disk-write-cache'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>320947</microcodeVersion> -- 2.16.2
On 04/04/2018 04:13 AM, Peter Krempa wrote:
QEMU translates the cache mode of a disk internally into 3 flags. 'write-cache' is a flag of the frontend while others are flag of the backing storage. Add capability which will allow expressing it via the frontend attribute.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 18 files changed, 22 insertions(+)
Will require merges with top of tree, but that's a trivial exercise. Curious though - the testing patch 6 of this series would seem to imply that this feature is only available for at least QEMU 2.12 on x64_64; however, the bit shows up starting in 2.7 on other arches, so since patch 5 only uses QEMU_CAPS_DISK_WRITE_CACHE to decide whether to use this or not, doesn't that imply earlier versions could use this? or that the testing should be different? John
On Wed, Apr 11, 2018 at 11:18:32 -0400, John Ferlan wrote:
On 04/04/2018 04:13 AM, Peter Krempa wrote:
QEMU translates the cache mode of a disk internally into 3 flags. 'write-cache' is a flag of the frontend while others are flag of the backing storage. Add capability which will allow expressing it via the frontend attribute.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 18 files changed, 22 insertions(+)
Will require merges with top of tree, but that's a trivial exercise.
Curious though - the testing patch 6 of this series would seem to imply that this feature is only available for at least QEMU 2.12 on x64_64;
I don't think that the DO_TEST_CAPS should be taken as an implication of when the feature was introduced. DO_TEST_CAPS is a better option for positive test cases than having to explicitly name capabilities as with DO_TEST.
however, the bit shows up starting in 2.7 on other arches, so since patch 5 only uses QEMU_CAPS_DISK_WRITE_CACHE to decide whether to use this or not, doesn't that imply earlier versions could use this? or that the testing should be different?
No. DO_TEST_CAPS is not implying anything.
On Wed, Apr 04, 2018 at 10:13:57AM +0200, Peter Krempa wrote:
QEMU translates the cache mode of a disk internally into 3 flags. 'write-cache' is a flag of the frontend while others are flag of the backing storage. Add capability which will allow expressing it via the frontend attribute.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 18 files changed, 22 insertions(+)
ACK Jano
The disk cache mode translates to various frontend and backend attributes for the qemu block layer. For the frontend device the 'writeback' parameter is used and provided as 'write-cache'. Implement this so that we can later switch to using -blockdev where we will not pass the cachemode directly any more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bbd3cd0a7d..83e263e9f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1882,6 +1882,30 @@ qemuCheckIOThreads(const virDomainDef *def, } +static int +qemuBuildDriveDevCacheStr(virDomainDiskDefPtr disk, + virBufferPtr buf, + virQEMUCapsPtr qemuCaps) +{ + bool wb; + + if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_DEFAULT) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DISK_WRITE_CACHE)) + return 0; + + if (qemuDomainDiskCachemodeFlags(disk->cachemode, &wb, NULL, NULL) < 0) + return -1; + + virBufferStrcat(buf, ",write-cache=", + virTristateSwitchTypeToString(virTristateSwitchFromBool(wb)), + NULL); + + return 0; +} + + char * qemuBuildDriveDevStr(const virDomainDef *def, virDomainDiskDefPtr disk, @@ -2194,6 +2218,9 @@ qemuBuildDriveDevStr(const virDomainDef *def, } } + if (qemuBuildDriveDevCacheStr(disk, &opt, qemuCaps) < 0) + goto error; + if (virBufferCheckError(&opt) < 0) goto error; -- 2.16.2
On 04/04/2018 04:13 AM, Peter Krempa wrote:
The disk cache mode translates to various frontend and backend attributes for the qemu block layer. For the frontend device the 'writeback' parameter is used and provided as 'write-cache'. Implement this so that we can later switch to using -blockdev where we will not pass the cachemode directly any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bbd3cd0a7d..83e263e9f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1882,6 +1882,30 @@ qemuCheckIOThreads(const virDomainDef *def, }
+static int +qemuBuildDriveDevCacheStr(virDomainDiskDefPtr disk, + virBufferPtr buf, + virQEMUCapsPtr qemuCaps) +{ + bool wb; + + if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_DEFAULT) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DISK_WRITE_CACHE)) + return 0;
As I noted in patch 4 - this is checking a bit that's defined in QEMU 2.7 on some arches, but the test in patch 6 (probably should be in this patch) would seem to indicate QEMU 2.12 is required.
+ + if (qemuDomainDiskCachemodeFlags(disk->cachemode, &wb, NULL, NULL) < 0) + return -1;
Wouldn't bother with -1 since we already know that cachemode will be valid (e.g. from my comments in patch 4). John
+ + virBufferStrcat(buf, ",write-cache=", + virTristateSwitchTypeToString(virTristateSwitchFromBool(wb)), + NULL); + + return 0; +} + + char * qemuBuildDriveDevStr(const virDomainDef *def, virDomainDiskDefPtr disk, @@ -2194,6 +2218,9 @@ qemuBuildDriveDevStr(const virDomainDef *def, } }
+ if (qemuBuildDriveDevCacheStr(disk, &opt, qemuCaps) < 0) + goto error; + if (virBufferCheckError(&opt) < 0) goto error;
On Wed, Apr 04, 2018 at 10:13:58AM +0200, Peter Krempa wrote:
The disk cache mode translates to various frontend and backend attributes for the qemu block layer. For the frontend device the 'writeback' parameter is used and provided as 'write-cache'. Implement this so that we can later switch to using -blockdev where we will not pass the cachemode directly any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
ACK Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvdata/disk-drive-write-cache.args | 45 ++++++++++++++++++++++ tests/qemuxml2argvdata/disk-drive-write-cache.xml | 45 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 91 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.args create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.xml diff --git a/tests/qemuxml2argvdata/disk-drive-write-cache.args b/tests/qemuxml2argvdata/disk-drive-write-cache.args new file mode 100644 index 0000000000..3973249d18 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-drive-write-cache.args @@ -0,0 +1,45 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc-i440fx-2.12,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-device lsi,id=scsi0,bus=pci.0,addr=0x2 \ +-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\ +cache=writeback \ +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1,\ +write-cache=on \ +-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-scsi0-0-0,\ +cache=none \ +-device scsi-hd,bus=scsi0.0,scsi-id=0,drive=drive-scsi0-0-0,id=scsi0-0-0,\ +write-cache=on \ +-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-virtio-disk0,\ +cache=writethrough \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0,write-cache=off \ +-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-usb-disk1,\ +cache=directsync \ +-device usb-storage,bus=usb.0,port=1,drive=drive-usb-disk1,id=usb-disk1,\ +removable=off,write-cache=off \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/disk-drive-write-cache.xml b/tests/qemuxml2argvdata/disk-drive-write-cache.xml new file mode 100644 index 0000000000..dc7bdd6050 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-drive-write-cache.xml @@ -0,0 +1,45 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writeback'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='scsi'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writethrough'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='directsync'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sdb' bus='usb'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c540ce2f50..b70780bf4e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -998,6 +998,7 @@ mymain(void) DO_TEST("disk-drive-cache-v2-wt", NONE); DO_TEST("disk-drive-cache-v2-wb", NONE); DO_TEST("disk-drive-cache-v2-none", NONE); + DO_TEST_CAPS("disk-drive-write-cache", "2.12.0"); DO_TEST("disk-drive-cache-directsync", QEMU_CAPS_DRIVE_CACHE_DIRECTSYNC); DO_TEST("disk-drive-cache-unsafe", -- 2.16.2
On 04/04/2018 04:13 AM, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvdata/disk-drive-write-cache.args | 45 ++++++++++++++++++++++ tests/qemuxml2argvdata/disk-drive-write-cache.xml | 45 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 91 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.args create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.xml
This should be merged with patch 5 - there's also no xml2xml test Also considering patch 2, all we're testing is x86_64 on 2.12 even though we have the feature available on other arches for other versions. I don't believe we have any existing test that sets cache= to anything other than 'none' (e.g. qemuDiskCacheV2 isn't really used or checked currently). Why wouldn't any of the existing cache=none would change too... OK other than the fact that you're only testing for 2.12 and beyond... Perhaps it would have been interesting to see the results of this test before the new capability and how it changes afterwards. John
diff --git a/tests/qemuxml2argvdata/disk-drive-write-cache.args b/tests/qemuxml2argvdata/disk-drive-write-cache.args new file mode 100644 index 0000000000..3973249d18 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-drive-write-cache.args @@ -0,0 +1,45 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc-i440fx-2.12,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-device lsi,id=scsi0,bus=pci.0,addr=0x2 \ +-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\ +cache=writeback \ +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1,\ +write-cache=on \ +-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-scsi0-0-0,\ +cache=none \ +-device scsi-hd,bus=scsi0.0,scsi-id=0,drive=drive-scsi0-0-0,id=scsi0-0-0,\ +write-cache=on \ +-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-virtio-disk0,\ +cache=writethrough \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0,write-cache=off \ +-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-usb-disk1,\ +cache=directsync \ +-device usb-storage,bus=usb.0,port=1,drive=drive-usb-disk1,id=usb-disk1,\ +removable=off,write-cache=off \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/disk-drive-write-cache.xml b/tests/qemuxml2argvdata/disk-drive-write-cache.xml new file mode 100644 index 0000000000..dc7bdd6050 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-drive-write-cache.xml @@ -0,0 +1,45 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writeback'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='scsi'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writethrough'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='directsync'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sdb' bus='usb'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c540ce2f50..b70780bf4e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -998,6 +998,7 @@ mymain(void) DO_TEST("disk-drive-cache-v2-wt", NONE); DO_TEST("disk-drive-cache-v2-wb", NONE); DO_TEST("disk-drive-cache-v2-none", NONE); + DO_TEST_CAPS("disk-drive-write-cache", "2.12.0"); DO_TEST("disk-drive-cache-directsync", QEMU_CAPS_DRIVE_CACHE_DIRECTSYNC); DO_TEST("disk-drive-cache-unsafe",
On Wed, Apr 11, 2018 at 11:20:43 -0400, John Ferlan wrote:
On 04/04/2018 04:13 AM, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvdata/disk-drive-write-cache.args | 45 ++++++++++++++++++++++ tests/qemuxml2argvdata/disk-drive-write-cache.xml | 45 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 91 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.args create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.xml
This should be merged with patch 5 - there's also no xml2xml test
I can squash this in to the previous patch, but I usually prefer having them separate when we are adding a lot of new lines. I don't think a XML2XML test is necessary here. This feature is not using any new XML configuration, but rather uses existing values to express the new configuration.
Also considering patch 2, all we're testing is x86_64 on 2.12 even though we have the feature available on other arches for other versions.
Well, given that prior to the ability to test it with real capabilities, we'd test it with a configuration which would not exist by listing the required capability bits in the surce file, the same would apply.
I don't believe we have any existing test that sets cache= to anything other than 'none' (e.g. qemuDiskCacheV2 isn't really used or checked currently).
Why wouldn't any of the existing cache=none would change too... OK other than the fact that you're only testing for 2.12 and beyond...
As said above. That's originating from the fact that our previous testing sucked and did not really compare to anything real-life. If I'd were to add a DO_TEST case using the capability explicitly the tests would not change in the same way as they did not change here, so the version used for capabilities does not really matter in any way here.
Perhaps it would have been interesting to see the results of this test before the new capability and how it changes afterwards.
I can shuffle them around if you want. But it then can't be merged with previous patch. Also until the feature is implemented, the test will not test what it is declaring to test.
On Wed, Apr 04, 2018 at 10:13:59AM +0200, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvdata/disk-drive-write-cache.args | 45 ++++++++++++++++++++++ tests/qemuxml2argvdata/disk-drive-write-cache.xml | 45 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 91 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.args create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.xml
ACK Jano
participants (3)
-
John Ferlan -
Ján Tomko -
Peter Krempa