[libvirt] [PATCH v2 0/7] qemu: Add support for 'write-cache' disk frontend argument (blockdev-add saga)
Version 2: - new and better testing against the newest capability file - reordered test case, so that the new parameter is visibly added Peter Krempa (7): tests: qemu: Add helper code to lookup most recent capability file tests: qemuxml2argv: Add infrastructure to pass output file suffix tests: qemuxml2argv: Add infrastructure for testing with real qemuCaps qemu: domain: Add helper for translating disk cachemode to qemu flags tests: qemuxml2argv: Test formatting of 'write-cache' parameter qemu: caps: Add capability for 'write-cache' parameter of disk frontends qemu: Format 'write-cache' parameter for disk frontends 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 + .../disk-drive-write-cache.new.args | 45 +++++++++++ tests/qemuxml2argvdata/disk-drive-write-cache.xml | 45 +++++++++++ tests/qemuxml2argvtest.c | 81 ++++++++++++++++++- tests/testutilsqemu.c | 94 ++++++++++++++++++++++ tests/testutilsqemu.h | 5 ++ 26 files changed, 397 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.new.args create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.xml -- 2.16.2
The helper iterates the directory with files for the capability test and looks up the most recent one for the given architecture. This will allow testing against the newest qemu capabilities so that we can catch regressions in behaviour more easily. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemu.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/testutilsqemu.h | 5 +++ 2 files changed, 99 insertions(+) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 9671a46f12..9dbcee6e3b 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -674,3 +674,97 @@ testQemuCapsSetGIC(virQEMUCapsPtr qemuCaps, } #endif + + +static bool +testQemuCapsIsNewerVersion(char **cur, + char **max) +{ + size_t i; + int rc; + + for (i = 0; i < 3; i++) { + rc = strlen(cur[i]) - strlen(max[i]); + + if (rc > 0) + return true; + + if (rc < 0) + return false; + + rc = strcmp(cur[i], max[i]); + + if (rc > 0) + return true; + + if (rc < 0) + return false; + } + + return false; +} + + +char * +testQemuGetNewestCapsForArch(const char *dirname, + const char *arch, + const char *suffix) +{ + struct dirent *ent; + int rc; + DIR *dir = NULL; + char *ret = NULL; + const char *tmp; + char **max = NULL; + size_t nmax = 0; + const char *maxname = NULL; + char **cur = NULL; + size_t ncur = 0; + + if (virDirOpen(&dir, dirname) < 0) + goto cleanup; + + while ((rc = virDirRead(dir, &ent, dirname)) > 0) { + virStringListFreeCount(cur, ncur); + cur = NULL; + ncur = 0; + + if (!(tmp = STRSKIP(ent->d_name, "caps_"))) + continue; + + if (!strstr(tmp, suffix) || !strstr(tmp, arch)) + continue; + + if (!(cur = virStringSplitCount(tmp, ".", 4, &ncur))) + goto cleanup; + + if (ncur != 4) { + VIR_TEST_DEBUG("skipping caps file '%s'\n", ent->d_name); + continue; + } + + if (!max || testQemuCapsIsNewerVersion(cur, max)) { + VIR_STEAL_PTR(max, cur); + maxname = ent->d_name; + nmax = ncur; + ncur = 0; + } + } + + if (rc < 0) + goto cleanup; + + if (!maxname) { + VIR_TEST_VERBOSE("failed to find capabilities for '%s' in '%s'\n", + arch, dirname); + goto cleanup; + } + + ignore_value(virAsprintf(&ret, "%s/%s", dirname, maxname)); + + cleanup: + virStringListFreeCount(max, nmax); + virStringListFreeCount(cur, ncur); + virDirClose(&dir); + return ret; +} diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index 7ae8324933..6490db4b95 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -39,4 +39,9 @@ int qemuTestCapsCacheInsert(virFileCachePtr cache, int testQemuCapsSetGIC(virQEMUCapsPtr qemuCaps, int gic); + +char *testQemuGetNewestCapsForArch(const char *dirname, + const char *arch, + const char *suffix); + #endif -- 2.16.2
On Wed, Apr 18, 2018 at 11:38:41AM +0200, Peter Krempa wrote:
The helper iterates the directory with files for the capability test and looks up the most recent one for the given architecture. This will allow testing against the newest qemu capabilities so that we can catch regressions in behaviour more easily.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemu.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/testutilsqemu.h | 5 +++ 2 files changed, 99 insertions(+)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 9671a46f12..9dbcee6e3b 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -674,3 +674,97 @@ testQemuCapsSetGIC(virQEMUCapsPtr qemuCaps, }
#endif + + +static bool +testQemuCapsIsNewerVersion(char **cur, + char **max) +{ + size_t i; + int rc; + + for (i = 0; i < 3; i++) { + rc = strlen(cur[i]) - strlen(max[i]); + + if (rc > 0) + return true; + + if (rc < 0) + return false; + + rc = strcmp(cur[i], max[i]); + + if (rc > 0) + return true; + + if (rc < 0) + return false; + } + + return false; +} + + +char * +testQemuGetNewestCapsForArch(const char *dirname, + const char *arch, + const char *suffix) +{ + struct dirent *ent; + int rc; + DIR *dir = NULL; + char *ret = NULL; + const char *tmp; + char **max = NULL; + size_t nmax = 0; + const char *maxname = NULL; + char **cur = NULL; + size_t ncur = 0; + + if (virDirOpen(&dir, dirname) < 0) + goto cleanup; + + while ((rc = virDirRead(dir, &ent, dirname)) > 0) { + virStringListFreeCount(cur, ncur); + cur = NULL; + ncur = 0; + + if (!(tmp = STRSKIP(ent->d_name, "caps_"))) + continue; + + if (!strstr(tmp, suffix) || !strstr(tmp, arch)) + continue; +
This helper assumes the prefix is "caps_" but leaves the suffix configurable. You can concatenate arch and suffix into some real_suffix, then 1. STRDUP(tmp, STRSKIP( "caps_")) 2. virFileStripSuffix(tmp, real_suffix) 3. Either use virParseVersionString and a plain numeric comparison, or import strverscmp from gnulib instead of subjecting the reader to testQemuCapsIsNewerVersion. Jano
+ if (!(cur = virStringSplitCount(tmp, ".", 4, &ncur))) + goto cleanup; + + if (ncur != 4) { + VIR_TEST_DEBUG("skipping caps file '%s'\n", ent->d_name); + continue; + } + + if (!max || testQemuCapsIsNewerVersion(cur, max)) { + VIR_STEAL_PTR(max, cur); + maxname = ent->d_name; + nmax = ncur; + ncur = 0; + } + } + + if (rc < 0) + goto cleanup; + + if (!maxname) { + VIR_TEST_VERBOSE("failed to find capabilities for '%s' in '%s'\n", + arch, dirname); + goto cleanup; + } + + ignore_value(virAsprintf(&ret, "%s/%s", dirname, maxname)); + + cleanup: + virStringListFreeCount(max, nmax); + virStringListFreeCount(cur, ncur); + virDirClose(&dir); + return ret; +} diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index 7ae8324933..6490db4b95 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -39,4 +39,9 @@ int qemuTestCapsCacheInsert(virFileCachePtr cache,
int testQemuCapsSetGIC(virQEMUCapsPtr qemuCaps, int gic); + +char *testQemuGetNewestCapsForArch(const char *dirname, + const char *arch, + const char *suffix); + #endif -- 2.16.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Apr 18, 2018 at 12:53:55 +0200, Ján Tomko wrote:
On Wed, Apr 18, 2018 at 11:38:41AM +0200, Peter Krempa wrote:
The helper iterates the directory with files for the capability test and looks up the most recent one for the given architecture. This will allow testing against the newest qemu capabilities so that we can catch regressions in behaviour more easily.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemu.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/testutilsqemu.h | 5 +++ 2 files changed, 99 insertions(+)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 9671a46f12..9dbcee6e3b 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -674,3 +674,97 @@ testQemuCapsSetGIC(virQEMUCapsPtr qemuCaps, }
#endif + + +static bool +testQemuCapsIsNewerVersion(char **cur, + char **max) +{ + size_t i; + int rc; + + for (i = 0; i < 3; i++) { + rc = strlen(cur[i]) - strlen(max[i]); + + if (rc > 0) + return true; + + if (rc < 0) + return false; + + rc = strcmp(cur[i], max[i]); + + if (rc > 0) + return true; + + if (rc < 0) + return false; + } + + return false; +} + + +char * +testQemuGetNewestCapsForArch(const char *dirname, + const char *arch, + const char *suffix) +{ + struct dirent *ent; + int rc; + DIR *dir = NULL; + char *ret = NULL; + const char *tmp; + char **max = NULL; + size_t nmax = 0; + const char *maxname = NULL; + char **cur = NULL; + size_t ncur = 0; + + if (virDirOpen(&dir, dirname) < 0) + goto cleanup; + + while ((rc = virDirRead(dir, &ent, dirname)) > 0) { + virStringListFreeCount(cur, ncur); + cur = NULL; + ncur = 0; + + if (!(tmp = STRSKIP(ent->d_name, "caps_"))) + continue; + + if (!strstr(tmp, suffix) || !strstr(tmp, arch)) + continue; +
This helper assumes the prefix is "caps_" but leaves the suffix configurable.
Suffix is configurable for cases when you might want to select the .replies file instead of the .xml file. This is planned to be used when I'll want to replace tests/qemuqapischema.json with the newest reply from the capabilitiestest. On the other hand, prefix is not really going to change, and if so, it will need to be fixed.
You can concatenate arch and suffix into some real_suffix, then 1. STRDUP(tmp, STRSKIP( "caps_")) 2. virFileStripSuffix(tmp, real_suffix) 3. Either use virParseVersionString and a plain numeric comparison, or import strverscmp from gnulib instead of subjecting the reader to testQemuCapsIsNewerVersion.
Okay.
To allow having more than one output file in the qemuxml2argvtest add a suffix member to the testInfo struct which will allow testing the same XML file with multiple capabilities files. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7732116604..31218652ce 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -270,6 +270,7 @@ typedef enum { struct testInfo { const char *name; + const char *suffix; virQEMUCapsPtr qemuCaps; const char *migrateFrom; int migrateFd; @@ -442,6 +443,7 @@ testCompareXMLToArgv(const void *data) char *args = NULL; char *migrateURI = NULL; char *actualargv = NULL; + const char *suffix = info->suffix; unsigned int flags = info->flags; unsigned int parseFlags = info->parseFlags; int ret = -1; @@ -458,6 +460,9 @@ testCompareXMLToArgv(const void *data) if (!(conn = virGetConnect())) goto cleanup; + if (!suffix) + suffix = ""; + conn->secretDriver = &fakeSecretDriver; conn->storageDriver = &fakeStorageDriver; @@ -472,8 +477,8 @@ testCompareXMLToArgv(const void *data) if (virAsprintf(&xml, "%s/qemuxml2argvdata/%s.xml", abs_srcdir, info->name) < 0 || - virAsprintf(&args, "%s/qemuxml2argvdata/%s.args", - abs_srcdir, info->name) < 0) + virAsprintf(&args, "%s/qemuxml2argvdata/%s%s.args", + abs_srcdir, info->name, suffix) < 0) goto cleanup; if (info->migrateFrom && @@ -657,7 +662,7 @@ mymain(void) parseFlags, gic, ...) \ do { \ static struct testInfo info = { \ - name, NULL, migrateFrom, migrateFd, (flags), parseFlags, \ + name, NULL, NULL, migrateFrom, migrateFd, (flags), parseFlags, \ false, NULL \ }; \ info.skipLegacyCPUs = skipLegacyCPUs; \ -- 2.16.2
On Wed, Apr 18, 2018 at 11:38:42AM +0200, Peter Krempa wrote:
To allow having more than one output file in the qemuxml2argvtest add a suffix member to the testInfo struct which will allow testing the same XML file with multiple capabilities files.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> 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 or allows to use the newest capabilities present. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 31218652ce..75a9d0b908 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -593,6 +593,7 @@ mymain(void) int ret = 0; char *fakerootdir; bool skipLegacyCPUs = false; + char *capsnew_x86_64 = NULL; if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { fprintf(stderr, "Out of memory\n"); @@ -658,6 +659,73 @@ mymain(void) if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0) return EXIT_FAILURE; + if (!(capsnew_x86_64 = testQemuGetNewestCapsForArch(abs_srcdir "/qemucapabilitiesdata", + "x86_64", ".xml"))) + return EXIT_FAILURE; + + VIR_TEST_VERBOSE("\ncaps x86_64: %s\n", capsnew_x86_64); + + +/** + * The following set of macros allows testing of XML -> argv conversion with a + * real set of capabilities gathered from a real qemu copy. It is desired to use + * these for positive test cases as it provides combinations of flags which + * can be met in real life. + * + * The capabilities are taken from the real capabilities stored in + * tests/qemucapabilitiesdata. + * + * It is suggested to use the DO_TEST_CAPS_NEW macro which always takes the + * newest capability set. In cases when the new capabilities would remove a + * feature for some reason, the test should be forked by using DO_TEST_CAPS_VER + * with the old version. New version of the capabilities can then be changed to + * the new format. + */ +# define DO_TEST_CAPS_INTERNAL(name, suffix, migrateFrom, flags, parseFlags, \ + gic, arch, capsfile) \ + do { \ + static struct testInfo info = { \ + name, "." suffix, 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), \ + capsfile))) \ + return EXIT_FAILURE; \ + if (virTestRun("QEMU XML-2-ARGV " name "." suffix, \ + testCompareXMLToArgv, &info) < 0) \ + ret = -1; \ + virObjectUnref(info.qemuCaps); \ + virObjectUnref(info.vm); \ + } while (0) + +# define TEST_CAPS_PATH abs_srcdir "/qemucapabilitiesdata/caps_" + +# define DO_TEST_CAPS_ARCH_VER_FULL(name, flags, parseFlags, gic, arch, ver) \ + DO_TEST_CAPS_INTERNAL(name, arch "-" ver, NULL, flags, parseFlags, gic, \ + arch, TEST_CAPS_PATH ver "." arch ".xml") + +# define DO_TEST_CAPS_ARCH_VER(name, gic, arch, ver) \ + DO_TEST_CAPS_ARCH_VER_FULL(name, 0, 0, gic, arch, ver) + +# define DO_TEST_CAPS_VER(name, ver) \ + DO_TEST_CAPS_ARCH_VER(name, GIC_NONE, "x86_64", ver) + +# define DO_TEST_CAPS_NEW(name) \ + DO_TEST_CAPS_INTERNAL(name, "new", NULL, 0, 0, GIC_NONE, "x86_64", capsnew_x86_64) + +/** + * The following test macros should be used only in cases when the tests require + * testing of some non-standard combination of capability flags + */ +# 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 { \ @@ -2767,6 +2835,7 @@ mymain(void) qemuTestDriverFree(&driver); VIR_FREE(fakerootdir); + VIR_FREE(capsnew_x86_64); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.16.2
On Wed, Apr 18, 2018 at 11:38:43AM +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 or allows to use the newest capabilities present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 31218652ce..75a9d0b908 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -593,6 +593,7 @@ mymain(void) int ret = 0; char *fakerootdir; bool skipLegacyCPUs = false; + char *capsnew_x86_64 = NULL;
if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { fprintf(stderr, "Out of memory\n"); @@ -658,6 +659,73 @@ mymain(void) if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0) return EXIT_FAILURE;
+ if (!(capsnew_x86_64 = testQemuGetNewestCapsForArch(abs_srcdir "/qemucapabilitiesdata", + "x86_64", ".xml"))) + return EXIT_FAILURE; + + VIR_TEST_VERBOSE("\ncaps x86_64: %s\n", capsnew_x86_64); + + +/** + * The following set of macros allows testing of XML -> argv conversion with a + * real set of capabilities gathered from a real qemu copy. It is desired to use + * these for positive test cases as it provides combinations of flags which + * can be met in real life. + * + * The capabilities are taken from the real capabilities stored in + * tests/qemucapabilitiesdata. + * + * It is suggested to use the DO_TEST_CAPS_NEW macro which always takes the + * newest capability set. In cases when the new capabilities would remove a + * feature for some reason, the test should be forked by using DO_TEST_CAPS_VER + * with the old version. New version of the capabilities can then be changed to + * the new format.
The main point of these unit tests so far was to check whether changes and refactors of the libvirt code do not affect the .args output for old QEMUs. Even introducting new capabilities could cause libvirt to take different code paths, therefore we should prefer DO_TEST_CAPS_VER. I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a new capability that needs to be handled by libvirt, the code author should introduce a new DO_TEST_CAPS_VER test for that. Otherwise it would just be checking whether QEMU did not break something for us. And since the soonest we update capabilities is at the time of QEMU freeze, that might be later than needed. With the comment fixed to encourage DO_TEST_CAPS_VER: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Wed, Apr 18, 2018 at 13:09:52 +0200, Ján Tomko wrote:
On Wed, Apr 18, 2018 at 11:38:43AM +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 or allows to use the newest capabilities present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 31218652ce..75a9d0b908 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -593,6 +593,7 @@ mymain(void) int ret = 0; char *fakerootdir; bool skipLegacyCPUs = false; + char *capsnew_x86_64 = NULL;
if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { fprintf(stderr, "Out of memory\n"); @@ -658,6 +659,73 @@ mymain(void) if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0) return EXIT_FAILURE;
+ if (!(capsnew_x86_64 = testQemuGetNewestCapsForArch(abs_srcdir "/qemucapabilitiesdata", + "x86_64", ".xml"))) + return EXIT_FAILURE; + + VIR_TEST_VERBOSE("\ncaps x86_64: %s\n", capsnew_x86_64); + + +/** + * The following set of macros allows testing of XML -> argv conversion with a + * real set of capabilities gathered from a real qemu copy. It is desired to use + * these for positive test cases as it provides combinations of flags which + * can be met in real life. + * + * The capabilities are taken from the real capabilities stored in + * tests/qemucapabilitiesdata. + * + * It is suggested to use the DO_TEST_CAPS_NEW macro which always takes the + * newest capability set. In cases when the new capabilities would remove a + * feature for some reason, the test should be forked by using DO_TEST_CAPS_VER + * with the old version. New version of the capabilities can then be changed to + * the new format.
The main point of these unit tests so far was to check whether changes and refactors of the libvirt code do not affect the .args output for old QEMUs.
Yes, but that's not the whole story.
Even introducting new capabilities could cause libvirt to take different code paths, therefore we should prefer DO_TEST_CAPS_VER.
This is actually the exact reason why I think DO_TEST_CAPS_NEW is better than DO_TEST_CAPS_VER. If we do changes to code used in multiple places which are gated on presence of a capability, the places using DO_TEST_CAPS_VER could change without us being able to catch that.
I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a new capability that needs to be handled by libvirt, the code author should introduce a new DO_TEST_CAPS_VER test for that.
In such reason, the author adding the code should fork the test by adding a DO_TEST_CAPS_VER along with the existing DO_TEST_CAPS_NEW and then add the new capability bit. With such approach it will be even visible which options changed.
Otherwise it would just be checking whether QEMU did not break something for us. And since the soonest we update capabilities is at the time of
Actually no. It will also check that something other will not break: https://www.redhat.com/archives/libvir-list/2018-April/msg01066.html tried to introduce a new property for the memory-backing-file object. This object is used in multiple places. The property is gated by a capability bit. Our tests were not able to catch, that this would add the property to the NVDIMM device which needs to persist the data, which would actually break it. While I agree that DO_TEST_CAPS_VER is very useful for checking old command line, I think that the true value is also in DO_TEST_CAPS_NEW when used together with DO_TEST_CAPS_VER, so when a new addition would change some tests using DO_TEST_CAPS_NEW we should fork them to cover both recent and old versions.
QEMU freeze, that might be later than needed.
With the comment fixed to encourage DO_TEST_CAPS_VER:
I can reword it but I disagree that DO_TEST_CAPS_VER should be used primarily.
On Wed, 2018-04-18 at 13:44 +0200, Peter Krempa wrote:
On Wed, Apr 18, 2018 at 13:09:52 +0200, Ján Tomko wrote:
Even introducting new capabilities could cause libvirt to take different code paths, therefore we should prefer DO_TEST_CAPS_VER.
This is actually the exact reason why I think DO_TEST_CAPS_NEW is better than DO_TEST_CAPS_VER. If we do changes to code used in multiple places which are gated on presence of a capability, the places using DO_TEST_CAPS_VER could change without us being able to catch that.
A note on naming: both the macro and the output file should use the world "latest" rather than "new" to be more informative and avoid any confusion.
I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a new capability that needs to be handled by libvirt, the code author should introduce a new DO_TEST_CAPS_VER test for that.
In such reason, the author adding the code should fork the test by adding a DO_TEST_CAPS_VER along with the existing DO_TEST_CAPS_NEW and then add the new capability bit. With such approach it will be even visible which options changed.
Otherwise it would just be checking whether QEMU did not break something for us. And since the soonest we update capabilities is at the time of
Actually no. It will also check that something other will not break:
https://www.redhat.com/archives/libvir-list/2018-April/msg01066.html tried to introduce a new property for the memory-backing-file object. This object is used in multiple places. The property is gated by a capability bit. Our tests were not able to catch, that this would add the property to the NVDIMM device which needs to persist the data, which would actually break it.
While I agree that DO_TEST_CAPS_VER is very useful for checking old command line, I think that the true value is also in DO_TEST_CAPS_NEW when used together with DO_TEST_CAPS_VER, so when a new addition would change some tests using DO_TEST_CAPS_NEW we should fork them to cover both recent and old versions.
I think using real-life capabilities instead of the syntetic data we use now is a step in the right direction. I'm still a bit uneasy about the "moving target" factor, though I've spent some time trying to come up with possible failure scenarios without much success... I think ideally with each new feature the author would introduce three tests: a negative one locked to a QEMU version that doesn't have the required bits, a positive one locked to a QEMU version that has them, and a positive one against the latest QEMU version. This way, we would make sure both that new QEMU versions work correctly with the feature and that changes in libvirt don't affect the behavior for existing QEMU versions. Does that sound reasonable? -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Apr 18, 2018 at 14:43:55 +0200, Andrea Bolognani wrote:
On Wed, 2018-04-18 at 13:44 +0200, Peter Krempa wrote:
On Wed, Apr 18, 2018 at 13:09:52 +0200, Ján Tomko wrote:
Even introducting new capabilities could cause libvirt to take different code paths, therefore we should prefer DO_TEST_CAPS_VER.
This is actually the exact reason why I think DO_TEST_CAPS_NEW is better than DO_TEST_CAPS_VER. If we do changes to code used in multiple places which are gated on presence of a capability, the places using DO_TEST_CAPS_VER could change without us being able to catch that.
A note on naming: both the macro and the output file should use the world "latest" rather than "new" to be more informative and avoid any confusion.
Oh right. I knew there is a better word for this but could not figure it out ;)
I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a new capability that needs to be handled by libvirt, the code author should introduce a new DO_TEST_CAPS_VER test for that.
In such reason, the author adding the code should fork the test by adding a DO_TEST_CAPS_VER along with the existing DO_TEST_CAPS_NEW and then add the new capability bit. With such approach it will be even visible which options changed.
Otherwise it would just be checking whether QEMU did not break something for us. And since the soonest we update capabilities is at the time of
Actually no. It will also check that something other will not break:
https://www.redhat.com/archives/libvir-list/2018-April/msg01066.html tried to introduce a new property for the memory-backing-file object. This object is used in multiple places. The property is gated by a capability bit. Our tests were not able to catch, that this would add the property to the NVDIMM device which needs to persist the data, which would actually break it.
While I agree that DO_TEST_CAPS_VER is very useful for checking old command line, I think that the true value is also in DO_TEST_CAPS_NEW when used together with DO_TEST_CAPS_VER, so when a new addition would change some tests using DO_TEST_CAPS_NEW we should fork them to cover both recent and old versions.
I think using real-life capabilities instead of the syntetic data we use now is a step in the right direction.
I'm still a bit uneasy about the "moving target" factor, though I've spent some time trying to come up with possible failure scenarios without much success...
There are few churn-scenarios, some avoidable, some not so much: - When adding something which is added for a lot of configurations or all of them (e.g. recent adding of the new seccomp code) the change will generate a lot of changes. - Machine type. We can't use any of the alias machine types in the XML files as they will change when new capabilities with new machine types are added. This can easily be avoided by using an explicit machine type. While the above may induce some churn in some scenarios, I think that the benefit of at least seeing that something changed in places where you did not expect any change may outweigh it.
I think ideally with each new feature the author would introduce three tests: a negative one locked to a QEMU version that doesn't have the required bits, a positive one locked to a QEMU version that has them, and a positive one against the latest QEMU version.
I'm considering whether it's necessary to have the one locked to the older-but-supporting version of qemu. My idea was that if somebody were to add test which would change anything, the test can be forked prior to that. But it seems that it may be a better idea to lock-in the changes right away.
This way, we would make sure both that new QEMU versions work correctly with the feature and that changes in libvirt don't affect the behavior for existing QEMU versions.
Does that sound reasonable?
Yes. I've actually modified the test code added in this series to do this.
On Wed, 2018-04-18 at 17:22 +0200, Peter Krempa wrote:
On Wed, Apr 18, 2018 at 14:43:55 +0200, Andrea Bolognani wrote:
I think using real-life capabilities instead of the syntetic data we use now is a step in the right direction.
I'm still a bit uneasy about the "moving target" factor, though I've spent some time trying to come up with possible failure scenarios without much success...
There are few churn-scenarios, some avoidable, some not so much:
- When adding something which is added for a lot of configurations or all of them (e.g. recent adding of the new seccomp code) the change will generate a lot of changes.
- Machine type. We can't use any of the alias machine types in the XML files as they will change when new capabilities with new machine types are added. This can easily be avoided by using an explicit machine type.
While the above may induce some churn in some scenarios, I think that the benefit of at least seeing that something changed in places where you did not expect any change may outweigh it.
I'm not really worried about churn, but about situations where changes to libvirt would make it behave wrongly with old versions of QEMU but correctly with newer ones, possibly with respect to features that are not directly touched by the changes, in ways unforeseen by the developer. Right now that's easy to spot because everything is locked to some well-defined set of capabilities, but if we started using moving targets for most tests we'd decrease coverage of older QEMU in favor of newer QEMU over time.
I think ideally with each new feature the author would introduce three tests: a negative one locked to a QEMU version that doesn't have the required bits, a positive one locked to a QEMU version that has them, and a positive one against the latest QEMU version.
I'm considering whether it's necessary to have the one locked to the older-but-supporting version of qemu.
My idea was that if somebody were to add test which would change anything, the test can be forked prior to that. But it seems that it may be a better idea to lock-in the changes right away.
I believe so. This would also make it so our test coverage always increases over time. Now, a crazy idea: what if the test author would define a baseline version QEMU where the test passes, and the test suite would automatically test all versions between that one and the latest? That would mean a lot of churn whenever we add new capabilities data, but it should be feasible to detect output files that do not change and turn them into symlinks to reduce diffs and save disk space. We would also need a way to run a test only in a range of versions, for negative testing. Too far fetched? :) -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Apr 19, 2018 at 09:29:55 +0200, Andrea Bolognani wrote:
On Wed, 2018-04-18 at 17:22 +0200, Peter Krempa wrote:
On Wed, Apr 18, 2018 at 14:43:55 +0200, Andrea Bolognani wrote:
I think using real-life capabilities instead of the syntetic data we use now is a step in the right direction.
I'm still a bit uneasy about the "moving target" factor, though I've spent some time trying to come up with possible failure scenarios without much success...
There are few churn-scenarios, some avoidable, some not so much:
- When adding something which is added for a lot of configurations or all of them (e.g. recent adding of the new seccomp code) the change will generate a lot of changes.
- Machine type. We can't use any of the alias machine types in the XML files as they will change when new capabilities with new machine types are added. This can easily be avoided by using an explicit machine type.
While the above may induce some churn in some scenarios, I think that the benefit of at least seeing that something changed in places where you did not expect any change may outweigh it.
I'm not really worried about churn, but about situations where changes to libvirt would make it behave wrongly with old versions of QEMU but correctly with newer ones, possibly with respect to features that are not directly touched by the changes, in ways unforeseen by the developer.
Well, that is actually exactly why we need to check against the latest capability set. If we have the locked-in feature bit set for testing, it will catch regressions which happen with that set of capabilities. Any regressing behaviour which would happen only with a newly added feature will not be caught, and those are the very sneaky ones.
Right now that's easy to spot because everything is locked to some well-defined set of capabilities, but if we started using moving
Well, only half of the scenarios is covered, as said above.
targets for most tests we'd decrease coverage of older QEMU in favor of newer QEMU over time.
I don't think this will happen if we fork the test for a certain version in cases when the output would change. If the output will not change we will not have to do anything. This will prevent having a lot of test files which don't do anything.
I think ideally with each new feature the author would introduce three tests: a negative one locked to a QEMU version that doesn't have the required bits, a positive one locked to a QEMU version that has them, and a positive one against the latest QEMU version.
I'm considering whether it's necessary to have the one locked to the older-but-supporting version of qemu.
My idea was that if somebody were to add test which would change anything, the test can be forked prior to that. But it seems that it may be a better idea to lock-in the changes right away.
I believe so. This would also make it so our test coverage always increases over time.
Now, a crazy idea: what if the test author would define a baseline version QEMU where the test passes, and the test suite would automatically test all versions between that one and the latest?
It certainly is techincally possible as we could e.g. do a range of versions for the macro rather than single version. I'm only worried that it's slightly too expensive. The good thing is, that when we'll fork the tests rigorously on any change we should get coverage for most code paths. The unfortunate part is that the qemuxml2argvtest has now 800 XML files for testing (some are invalid though). If we introduce cartesian testing with all capability dumps, it will expand the test case count 12 fold which will tend to increase over time. (Thankfully Jano deleted a lot of old crap though)
That would mean a lot of churn whenever we add new capabilities data, but it should be feasible to detect output files that do not change and turn them into symlinks to reduce diffs and save disk space.
Actually if a test is stable (which requires it to be as minimal as possible) in a range of versions, adding new capabilities should be for free. We can test all versions in a range against the same output file. It should only change when adding a new feature. This only requires forking the test file if a change modifies anything retroactively.
We would also need a way to run a test only in a range of versions, for negative testing.
Well, the same as for positive testing. If we have infrastructure to test a version range, then this should be possible as well. I'm not sure though whether it's worth it at all.
Too far fetched? :)
Slightly. But certainly has some value. Unfortunately I can't put in much more time besides this initial idea, since I'm busy with the blockdev stuff.
On Wed, Apr 18, 2018 at 01:44:23PM +0200, Peter Krempa wrote:
On Wed, Apr 18, 2018 at 13:09:52 +0200, Ján Tomko wrote:
On Wed, Apr 18, 2018 at 11:38:43AM +0200, Peter Krempa wrote:
[ ... yaba daba doo ... ]
I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a new capability that needs to be handled by libvirt, the code author should introduce a new DO_TEST_CAPS_VER test for that.
In such reason, the author adding the code should fork the test by adding a DO_TEST_CAPS_VER along with the existing DO_TEST_CAPS_NEW and then add the new capability bit. With such approach it will be even visible which options changed.
Okay.
Otherwise it would just be checking whether QEMU did not break something for us. And since the soonest we update capabilities is at the time of
Actually no. It will also check that something other will not break:
https://www.redhat.com/archives/libvir-list/2018-April/msg01066.html tried to introduce a new property for the memory-backing-file object. This object is used in multiple places. The property is gated by a capability bit. Our tests were not able to catch, that this would add the property to the NVDIMM device which needs to persist the data, which would actually break it.
Seems _NEW wins here.
While I agree that DO_TEST_CAPS_VER is very useful for checking old command line, I think that the true value is also in DO_TEST_CAPS_NEW when used together with DO_TEST_CAPS_VER, so when a new addition would change some tests using DO_TEST_CAPS_NEW we should fork them to cover both recent and old versions.
QEMU freeze, that might be later than needed.
With the comment fixed to encourage DO_TEST_CAPS_VER:
I can reword it but I disagree that DO_TEST_CAPS_VER should be used primarily.
ACK then, as long as you incorporate Andrea's suggestion to call it _LATEST 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 4c4a9a428d..79eb2b7221 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11869,6 +11869,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 40d9a6f247..2c27dfb9f3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -994,4 +994,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 Wed, Apr 18, 2018 at 11:38:44AM +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(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../disk-drive-write-cache.new.args | 43 +++++++++++++++++++++ tests/qemuxml2argvdata/disk-drive-write-cache.xml | 45 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 89 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.new.args create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.xml diff --git a/tests/qemuxml2argvdata/disk-drive-write-cache.new.args b/tests/qemuxml2argvdata/disk-drive-write-cache.new.args new file mode 100644 index 0000000000..90414a100f --- /dev/null +++ b/tests/qemuxml2argvdata/disk-drive-write-cache.new.args @@ -0,0 +1,43 @@ +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 \ +-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 \ +-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 \ +-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 \ +-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 75a9d0b908..2fa020675f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1015,6 +1015,7 @@ mymain(void) DO_TEST("disk-drive-cache-v2-none", NONE); DO_TEST("disk-drive-cache-directsync", NONE); DO_TEST("disk-drive-cache-unsafe", NONE); + DO_TEST_CAPS_NEW("disk-drive-write-cache"); DO_TEST("disk-drive-network-nbd", NONE); DO_TEST("disk-drive-network-nbd-export", NONE); DO_TEST("disk-drive-network-nbd-ipv6", NONE); -- 2.16.2
On Wed, Apr 18, 2018 at 11:38:45AM +0200, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../disk-drive-write-cache.new.args | 43 +++++++++++++++++++++ tests/qemuxml2argvdata/disk-drive-write-cache.xml | 45 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 89 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.new.args create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.xml
diff --git a/tests/qemuxml2argvdata/disk-drive-write-cache.new.args b/tests/qemuxml2argvdata/disk-drive-write-cache.new.args new file mode 100644 index 0000000000..90414a100f --- /dev/null +++ b/tests/qemuxml2argvdata/disk-drive-write-cache.new.args @@ -0,0 +1,43 @@ +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 \ +-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 \ +-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 \ +-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 \ +-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 75a9d0b908..2fa020675f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1015,6 +1015,7 @@ mymain(void) DO_TEST("disk-drive-cache-v2-none", NONE); DO_TEST("disk-drive-cache-directsync", NONE); DO_TEST("disk-drive-cache-unsafe", NONE); + DO_TEST_CAPS_NEW("disk-drive-write-cache");
DO_TEST_VER with the earliest supported version would be IMO more beneficial. DO_TEST_CAPS_NEW might be nice to have. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
DO_TEST("disk-drive-network-nbd", NONE); DO_TEST("disk-drive-network-nbd-export", NONE); DO_TEST("disk-drive-network-nbd-ipv6", NONE); -- 2.16.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Apr 18, 2018 at 13:17:00 +0200, Ján Tomko wrote:
On Wed, Apr 18, 2018 at 11:38:45AM +0200, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../disk-drive-write-cache.new.args | 43 +++++++++++++++++++++ tests/qemuxml2argvdata/disk-drive-write-cache.xml | 45 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 89 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.new.args create mode 100644 tests/qemuxml2argvdata/disk-drive-write-cache.xml
[...]
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 75a9d0b908..2fa020675f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1015,6 +1015,7 @@ mymain(void) DO_TEST("disk-drive-cache-v2-none", NONE); DO_TEST("disk-drive-cache-directsync", NONE); DO_TEST("disk-drive-cache-unsafe", NONE); + DO_TEST_CAPS_NEW("disk-drive-write-cache");
DO_TEST_VER with the earliest supported version would be IMO more beneficial. DO_TEST_CAPS_NEW might be nice to have.
Actually, to prove my point about the testing infrastructure I can do DO_TEST_VER with a version not supporting the flag along with DO_TEST_CAPS_NEW where it will change.
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 1dae540962..3c5daf3b44 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -468,6 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-tablet-ccw", "qcow2-luks", "pcie-pci-bridge", + "disk-write-cache", ); @@ -1116,6 +1117,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[] = { @@ -1153,11 +1155,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[] = { @@ -1189,6 +1193,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 7d000b1513..d1b53994e0 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -452,6 +452,7 @@ typedef enum { QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */ QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */ QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */ + 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 88616e57f0..527b425dee 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml @@ -152,6 +152,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 91f80a277f..3ac6be2d88 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml @@ -151,6 +151,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 c599b66f56..b682d36c93 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml @@ -112,6 +112,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 9e95b68457..8dd30ccbcf 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml @@ -195,6 +195,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 cf0648fbfd..801f9fa328 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -116,6 +116,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 34055b4e71..349d95914f 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -154,6 +154,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> + <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 d19f35dbba..9ef7ad0f87 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -151,6 +151,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 24220943f2..4e3f000e83 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -116,6 +116,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 3d79d7f309..4675c5f180 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -192,6 +192,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> + <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 a585af453c..34eed23694 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml @@ -102,6 +102,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 55c121c596..ee4b25d7fd 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml @@ -175,6 +175,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 1ac60bb403..21046c0546 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml @@ -104,6 +104,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 831a768977..c4801ac23a 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml @@ -177,6 +177,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 4998edf7a0..73f42a7392 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml @@ -143,6 +143,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 d29994bbfd..30fce6ecf1 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml @@ -107,6 +107,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 d813a96a13..e1dc257a75 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml @@ -190,6 +190,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 Wed, Apr 18, 2018 at 11:38:46AM +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(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> 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 ++++++++++++++++++++++ .../disk-drive-write-cache.new.args | 10 ++++---- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a1a9e91e49..08c6e4faac 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1822,6 +1822,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, @@ -2134,6 +2158,9 @@ qemuBuildDriveDevStr(const virDomainDef *def, } } + if (qemuBuildDriveDevCacheStr(disk, &opt, qemuCaps) < 0) + goto error; + if (virBufferCheckError(&opt) < 0) goto error; diff --git a/tests/qemuxml2argvdata/disk-drive-write-cache.new.args b/tests/qemuxml2argvdata/disk-drive-write-cache.new.args index 90414a100f..3973249d18 100644 --- a/tests/qemuxml2argvdata/disk-drive-write-cache.new.args +++ b/tests/qemuxml2argvdata/disk-drive-write-cache.new.args @@ -28,16 +28,18 @@ server,nowait \ -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 \ +-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 \ +-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 \ +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 \ +removable=off,write-cache=off \ -msg timestamp=on -- 2.16.2
On Wed, Apr 18, 2018 at 11:38:47AM +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 ++++++++++++++++++++++ .../disk-drive-write-cache.new.args | 10 ++++---- 2 files changed, 33 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Andrea Bolognani -
Ján Tomko -
Peter Krempa