[libvirt] [PATCH 0/8] tests: Improve QEMU capability testing
This series removes the need to fiddle with test programs every time a new .replies file is added to the test suite, and also completely removes the possibility of missing on some test coverage because only one of the two test programs is updated. Andrea Bolognani (8): tests: Introduce testQemuDataInit() and testQemuDataReset() tests: Move ret into testQemuData tests: Move data directories into testQemuData tests: Use virAsprintf() to build titles tests: Move code from DO_TEST() to doCapsTest() tests: Add testutilsqemu dependency for qemucaps2xmltest tests: Introduce testQemuCapsIterate() tests: Use testQemuCapsIterate() tests/Makefile.am | 1 + tests/qemucapabilitiestest.c | 123 ++++++++++++++++++----------------- tests/qemucaps2xmltest.c | 104 ++++++++++++++--------------- tests/testutilsqemu.c | 53 +++++++++++++++ tests/testutilsqemu.h | 8 +++ 5 files changed, 179 insertions(+), 110 deletions(-) -- 2.20.1
These functions don't do anything too interesting right now, but will be extended significantly later on. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 25 ++++++++++++++++++++++--- tests/qemucaps2xmltest.c | 16 ++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index 8d47133e6f..882fa57485 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -40,6 +40,23 @@ struct _testQemuData { }; +static int +testQemuDataInit(testQemuDataPtr data) +{ + if (qemuTestDriverInit(&data->driver) < 0) + return -1; + + return 0; +} + + +static void +testQemuDataReset(testQemuDataPtr data) +{ + qemuTestDriverFree(&data->driver); +} + + static int testQemuCaps(const void *opaque) { @@ -164,12 +181,14 @@ mymain(void) return EXIT_AM_SKIP; #endif - if (virThreadInitialize() < 0 || - qemuTestDriverInit(&data.driver) < 0) + if (virThreadInitialize() < 0) return EXIT_FAILURE; virEventRegisterDefaultImpl(); + if (testQemuDataInit(&data) < 0) + return EXIT_FAILURE; + #define DO_TEST(arch, name) \ do { \ data.archName = arch; \ @@ -227,7 +246,7 @@ mymain(void) * "tests/qemucapsfixreplies foo.replies" to fix the replies ids. */ - qemuTestDriverFree(&data.driver); + testQemuDataReset(&data); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index 0d9b4e679a..5cc9fb635b 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -32,6 +32,17 @@ struct _testQemuData { const char *archName; }; +static int +testQemuDataInit(testQemuDataPtr data ATTRIBUTE_UNUSED) +{ + return 0; +} + +static void +testQemuDataReset(testQemuDataPtr data ATTRIBUTE_UNUSED) +{ +} + static virQEMUCapsPtr testQemuGetCaps(char *caps) { @@ -176,6 +187,9 @@ mymain(void) virEventRegisterDefaultImpl(); + if (testQemuDataInit(&data) < 0) + return EXIT_FAILURE; + #define DO_TEST(arch, name) \ data.archName = arch; \ data.base = name; \ @@ -220,6 +234,8 @@ mymain(void) DO_TEST("riscv64", "caps_3.0.0"); DO_TEST("riscv64", "caps_4.0.0"); + testQemuDataReset(&data); + return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.20.1
On Thu, Mar 07, 2019 at 16:44:30 +0100, Andrea Bolognani wrote:
These functions don't do anything too interesting right now, but will be extended significantly later on.
The state after all patches applied does not really look "significant"
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 25 ++++++++++++++++++++++--- tests/qemucaps2xmltest.c | 16 ++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index 8d47133e6f..882fa57485 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -40,6 +40,23 @@ struct _testQemuData { };
+static int +testQemuDataInit(testQemuDataPtr data) +{ + if (qemuTestDriverInit(&data->driver) < 0) + return -1; + + return 0; +} + + +static void +testQemuDataReset(testQemuDataPtr data) +{ + qemuTestDriverFree(&data->driver); +} + + static int testQemuCaps(const void *opaque) { @@ -164,12 +181,14 @@ mymain(void) return EXIT_AM_SKIP; #endif
- if (virThreadInitialize() < 0 || - qemuTestDriverInit(&data.driver) < 0) + if (virThreadInitialize() < 0) return EXIT_FAILURE;
virEventRegisterDefaultImpl();
+ if (testQemuDataInit(&data) < 0) + return EXIT_FAILURE; + #define DO_TEST(arch, name) \ do { \ data.archName = arch; \ @@ -227,7 +246,7 @@ mymain(void) * "tests/qemucapsfixreplies foo.replies" to fix the replies ids. */
- qemuTestDriverFree(&data.driver); + testQemuDataReset(&data);
return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index 0d9b4e679a..5cc9fb635b 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -32,6 +32,17 @@ struct _testQemuData { const char *archName; };
+static int +testQemuDataInit(testQemuDataPtr data ATTRIBUTE_UNUSED) +{ + return 0; +} + +static void +testQemuDataReset(testQemuDataPtr data ATTRIBUTE_UNUSED) +{ +}
This function is never touched again in this series.
+ static virQEMUCapsPtr testQemuGetCaps(char *caps) { @@ -176,6 +187,9 @@ mymain(void)
virEventRegisterDefaultImpl();
+ if (testQemuDataInit(&data) < 0) + return EXIT_FAILURE; + #define DO_TEST(arch, name) \ data.archName = arch; \ data.base = name; \ @@ -220,6 +234,8 @@ mymain(void) DO_TEST("riscv64", "caps_3.0.0"); DO_TEST("riscv64", "caps_4.0.0");
+ testQemuDataReset(&data); + return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; }
-- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2019-03-13 at 09:44 +0100, Peter Krempa wrote:
On Thu, Mar 07, 2019 at 16:44:30 +0100, Andrea Bolognani wrote:
These functions don't do anything too interesting right now, but will be extended significantly later on.
The state after all patches applied does not really look "significant"
Fair enough, I'll drop the inaccurate adjective :) [...]
+static void +testQemuDataReset(testQemuDataPtr data ATTRIBUTE_UNUSED) +{ +}
This function is never touched again in this series.
Yeah, there's no memory that needs to be released for this specific structure. I can drop the function if you feel strongly about that, but I'd rather keep it for consistency with the other test program. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 13, 2019 at 10:09:17 +0100, Andrea Bolognani wrote:
On Wed, 2019-03-13 at 09:44 +0100, Peter Krempa wrote:
On Thu, Mar 07, 2019 at 16:44:30 +0100, Andrea Bolognani wrote:
These functions don't do anything too interesting right now, but will be extended significantly later on.
The state after all patches applied does not really look "significant"
Fair enough, I'll drop the inaccurate adjective :)
[...]
+static void +testQemuDataReset(testQemuDataPtr data ATTRIBUTE_UNUSED) +{ +}
This function is never touched again in this series.
Yeah, there's no memory that needs to be released for this specific structure. I can drop the function if you feel strongly about that, but I'd rather keep it for consistency with the other test program.
Actually I don't see the reason for anything in this patch. Each function is called exactly once, so splitting it out does not make much sense without any further justification. The need to reset data->ret to 0 is not enough of a justification.
On Wed, 2019-03-13 at 10:12 +0100, Peter Krempa wrote:
Actually I don't see the reason for anything in this patch. Each function is called exactly once, so splitting it out does not make much sense without any further justification.
The need to reset data->ret to 0 is not enough of a justification.
Better code organization: instead of having a bunch of initialization and cleanup steps sprinkled throughout main(), we give them their own functions and keep main() simple, clean and focused. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 13, 2019 at 10:29:52 +0100, Andrea Bolognani wrote:
On Wed, 2019-03-13 at 10:12 +0100, Peter Krempa wrote:
Actually I don't see the reason for anything in this patch. Each function is called exactly once, so splitting it out does not make much sense without any further justification.
The need to reset data->ret to 0 is not enough of a justification.
Better code organization: instead of having a bunch of initialization and cleanup steps sprinkled throughout main(), we give them their own functions and keep main() simple, clean and focused.
Since I've ACK'd passing in of the data directory via the structure, this makes sense. Just get rid of the unused function.
This is not particularly useful right now, but will allow us to refactor some functionality later on. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 10 ++++++---- tests/qemucaps2xmltest.c | 11 ++++++----- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index 882fa57485..0f875f9e24 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -37,6 +37,7 @@ struct _testQemuData { virQEMUDriver driver; const char *archName; const char *base; + int ret; }; @@ -46,6 +47,8 @@ testQemuDataInit(testQemuDataPtr data) if (qemuTestDriverInit(&data->driver) < 0) return -1; + data->ret = 0; + return 0; } @@ -173,7 +176,6 @@ testQemuCapsCopy(const void *opaque) static int mymain(void) { - int ret = 0; testQemuData data; #if !WITH_YAJL @@ -194,10 +196,10 @@ mymain(void) data.archName = arch; \ data.base = name; \ if (virTestRun(name "(" arch ")", testQemuCaps, &data) < 0) \ - ret = -1; \ + data.ret = -1; \ if (virTestRun("copy " name "(" arch ")", \ testQemuCapsCopy, &data) < 0) \ - ret = -1; \ + data.ret = -1; \ } while (0) /* Keep this in sync with qemucaps2xmltest */ @@ -248,7 +250,7 @@ mymain(void) testQemuDataReset(&data); - return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; + return (data.ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } VIR_TEST_MAIN(mymain) diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index 5cc9fb635b..e9b0b11e35 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -30,11 +30,14 @@ typedef testQemuData *testQemuDataPtr; struct _testQemuData { const char *base; const char *archName; + int ret; }; static int -testQemuDataInit(testQemuDataPtr data ATTRIBUTE_UNUSED) +testQemuDataInit(testQemuDataPtr data) { + data->ret = 0; + return 0; } @@ -173,8 +176,6 @@ testQemuCapsXML(const void *opaque) static int mymain(void) { - int ret = 0; - testQemuData data; #if !WITH_YAJL @@ -194,7 +195,7 @@ mymain(void) data.archName = arch; \ data.base = name; \ if (virTestRun(name "(" arch ")", testQemuCapsXML, &data) < 0) \ - ret = -1 + data.ret = -1 /* Keep this in sync with qemucapabilitiestest */ DO_TEST("x86_64", "caps_1.5.3"); @@ -236,7 +237,7 @@ mymain(void) testQemuDataReset(&data); - return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; + return (data.ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } VIR_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/qemucaps2xmlmock.so") -- 2.20.1
On Thu, Mar 07, 2019 at 16:44:31 +0100, Andrea Bolognani wrote:
This is not particularly useful right now, but will allow us to refactor some functionality later on.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 10 ++++++---- tests/qemucaps2xmltest.c | 11 ++++++----- 2 files changed, 12 insertions(+), 9 deletions(-)
ACK
This removes a little duplication right away, and more importantly will allow us to perform some interesting refactoring later on. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 15 +++++++++------ tests/qemucaps2xmltest.c | 13 +++++++++---- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index 0f875f9e24..e3c6681dd4 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -35,6 +35,7 @@ typedef struct _testQemuData testQemuData; typedef testQemuData *testQemuDataPtr; struct _testQemuData { virQEMUDriver driver; + const char *dataDir; const char *archName; const char *base; int ret; @@ -47,6 +48,8 @@ testQemuDataInit(testQemuDataPtr data) if (qemuTestDriverInit(&data->driver) < 0) return -1; + data->dataDir = abs_srcdir "/qemucapabilitiesdata"; + data->ret = 0; return 0; @@ -73,10 +76,10 @@ testQemuCaps(const void *opaque) unsigned int fakeMicrocodeVersion = 0; const char *p; - if (virAsprintf(&repliesFile, "%s/qemucapabilitiesdata/%s.%s.replies", - abs_srcdir, data->base, data->archName) < 0 || - virAsprintf(&capsFile, "%s/qemucapabilitiesdata/%s.%s.xml", - abs_srcdir, data->base, data->archName) < 0) + if (virAsprintf(&repliesFile, "%s/%s.%s.replies", + data->dataDir, data->base, data->archName) < 0 || + virAsprintf(&capsFile, "%s/%s.%s.xml", + data->dataDir, data->base, data->archName) < 0) goto cleanup; if (!(mon = qemuMonitorTestNewFromFileFull(repliesFile, &data->driver, NULL))) @@ -141,8 +144,8 @@ testQemuCapsCopy(const void *opaque) virQEMUCapsPtr copy = NULL; char *actual = NULL; - if (virAsprintf(&capsFile, "%s/qemucapabilitiesdata/%s.%s.xml", - abs_srcdir, data->base, data->archName) < 0) + if (virAsprintf(&capsFile, "%s/%s.%s.xml", + data->dataDir, data->base, data->archName) < 0) goto cleanup; if (!(caps = virCapabilitiesNew(virArchFromString(data->archName), diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index e9b0b11e35..46d2ce8b44 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -28,6 +28,8 @@ typedef struct _testQemuData testQemuData; typedef testQemuData *testQemuDataPtr; struct _testQemuData { + const char *inputDir; + const char *outputDir; const char *base; const char *archName; int ret; @@ -36,6 +38,9 @@ struct _testQemuData { static int testQemuDataInit(testQemuDataPtr data) { + data->inputDir = abs_srcdir "/qemucapabilitiesdata"; + data->outputDir = abs_srcdir "/qemucaps2xmloutdata"; + data->ret = 0; return 0; @@ -142,12 +147,12 @@ testQemuCapsXML(const void *opaque) char *capsXml = NULL; virCapsPtr capsProvided = NULL; - if (virAsprintf(&xmlFile, "%s/qemucaps2xmloutdata/caps.%s.xml", - abs_srcdir, data->archName) < 0) + if (virAsprintf(&xmlFile, "%s/caps.%s.xml", + data->outputDir, data->archName) < 0) goto cleanup; - if (virAsprintf(&capsFile, "%s/qemucapabilitiesdata/%s.%s.xml", - abs_srcdir, data->base, data->archName) < 0) + if (virAsprintf(&capsFile, "%s/%s.%s.xml", + data->inputDir, data->base, data->archName) < 0) goto cleanup; if (virTestLoadFile(capsFile, &capsData) < 0) -- 2.20.1
On Thu, Mar 07, 2019 at 16:44:32 +0100, Andrea Bolognani wrote:
This removes a little duplication right away, and more importantly will allow us to perform some interesting refactoring later on.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 15 +++++++++------ tests/qemucaps2xmltest.c | 13 +++++++++---- 2 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index 0f875f9e24..e3c6681dd4 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c
[...]
@@ -47,6 +48,8 @@ testQemuDataInit(testQemuDataPtr data) if (qemuTestDriverInit(&data->driver) < 0) return -1;
+ data->dataDir = abs_srcdir "/qemucapabilitiesdata"; +
I'm not entirely persuaded that you need to pass this constant in via the data structure. Even after the last patch the value is never modified. Could you please elaborate on the refactorings that this will allow? Same applies for the other data structure modified in this patch.
On Wed, 2019-03-13 at 09:41 +0100, Peter Krempa wrote:
On Thu, Mar 07, 2019 at 16:44:32 +0100, Andrea Bolognani wrote:
This removes a little duplication right away, and more importantly will allow us to perform some interesting refactoring later on. [...] @@ -47,6 +48,8 @@ testQemuDataInit(testQemuDataPtr data) if (qemuTestDriverInit(&data->driver) < 0) return -1;
+ data->dataDir = abs_srcdir "/qemucapabilitiesdata"; +
I'm not entirely persuaded that you need to pass this constant in via the data structure. Even after the last patch the value is never modified. Could you please elaborate on the refactorings that this will allow?
I'll admit I kinda copy-pasted commit messages around, so that's probably why the one for this patch sounds a bit hyperbolic :) Anyway, by the end of the series we'll have to pass dataDir to testQemuCapsIterate() in addition to using it to build the path for both the input and output file, so it makes sense to store it into a variable instead of building it from abs_srcdir plus the directory name two or more times. As for why it's stored in the structure, I could easily have used a global variable instead, but this approach seemed cleaner, especially considering that we're already using the structure to store data that is similar in scope (the driver). -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 13, 2019 at 10:18:52 +0100, Andrea Bolognani wrote:
On Wed, 2019-03-13 at 09:41 +0100, Peter Krempa wrote:
On Thu, Mar 07, 2019 at 16:44:32 +0100, Andrea Bolognani wrote:
This removes a little duplication right away, and more importantly will allow us to perform some interesting refactoring later on. [...] @@ -47,6 +48,8 @@ testQemuDataInit(testQemuDataPtr data) if (qemuTestDriverInit(&data->driver) < 0) return -1;
+ data->dataDir = abs_srcdir "/qemucapabilitiesdata"; +
I'm not entirely persuaded that you need to pass this constant in via the data structure. Even after the last patch the value is never modified. Could you please elaborate on the refactorings that this will allow?
I'll admit I kinda copy-pasted commit messages around, so that's probably why the one for this patch sounds a bit hyperbolic :)
Anyway, by the end of the series we'll have to pass dataDir to testQemuCapsIterate() in addition to using it to build the path for both the input and output file, so it makes sense to store it into a variable instead of building it from abs_srcdir plus the directory name two or more times.
As for why it's stored in the structure, I could easily have used a global variable instead, but this approach seemed cleaner, especially considering that we're already using the structure to store data that is similar in scope (the driver).
Well, I agree that we should pre-construct the string rather than constructing it multiple times, but since every use of that string is inside the function which does the testing there's no point passing it in via the structure.
On Wed, 2019-03-13 at 10:22 +0100, Peter Krempa wrote:
On Wed, Mar 13, 2019 at 10:18:52 +0100, Andrea Bolognani wrote:
Anyway, by the end of the series we'll have to pass dataDir to testQemuCapsIterate() in addition to using it to build the path for both the input and output file, so it makes sense to store it into a variable instead of building it from abs_srcdir plus the directory name two or more times.
As for why it's stored in the structure, I could easily have used a global variable instead, but this approach seemed cleaner, especially considering that we're already using the structure to store data that is similar in scope (the driver).
Well, I agree that we should pre-construct the string rather than constructing it multiple times, but since every use of that string is inside the function which does the testing there's no point passing it in via the structure.
As mentioned above, one of the uses is as an argument to testQemuCapsIterate(), which needs to be called from main() rather than from doCapsTest(). So the alternatives are building the string multiple times, using a global variable or doing what this patch does. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 13, 2019 at 10:32:59 +0100, Andrea Bolognani wrote:
On Wed, 2019-03-13 at 10:22 +0100, Peter Krempa wrote:
On Wed, Mar 13, 2019 at 10:18:52 +0100, Andrea Bolognani wrote:
Anyway, by the end of the series we'll have to pass dataDir to testQemuCapsIterate() in addition to using it to build the path for both the input and output file, so it makes sense to store it into a variable instead of building it from abs_srcdir plus the directory name two or more times.
As for why it's stored in the structure, I could easily have used a global variable instead, but this approach seemed cleaner, especially considering that we're already using the structure to store data that is similar in scope (the driver).
Well, I agree that we should pre-construct the string rather than constructing it multiple times, but since every use of that string is inside the function which does the testing there's no point passing it in via the structure.
As mentioned above, one of the uses is as an argument to testQemuCapsIterate(), which needs to be called from main() rather than from doCapsTest().
Um, my bad. I did not read the commit message thoroughly apparently. ACK
We're using static string concatenation at the moment, but that will no longer be a possibility in a bit. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 11 ++++++++--- tests/qemucaps2xmltest.c | 13 +++++++++---- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index e3c6681dd4..222ac05d79 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -196,12 +196,17 @@ mymain(void) #define DO_TEST(arch, name) \ do { \ + VIR_AUTOFREE(char *) title = NULL; \ + VIR_AUTOFREE(char *) copyTitle = NULL; \ + if (virAsprintf(&title, "%s (%s)", name, arch) < 0 || \ + virAsprintf(©Title, "copy %s (%s)", name, arch) < 0) { \ + return -EXIT_FAILURE; \ + } \ data.archName = arch; \ data.base = name; \ - if (virTestRun(name "(" arch ")", testQemuCaps, &data) < 0) \ + if (virTestRun(title, testQemuCaps, &data) < 0) \ data.ret = -1; \ - if (virTestRun("copy " name "(" arch ")", \ - testQemuCapsCopy, &data) < 0) \ + if (virTestRun(copyTitle, testQemuCapsCopy, &data) < 0) \ data.ret = -1; \ } while (0) diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index 46d2ce8b44..be460b42f8 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -197,10 +197,15 @@ mymain(void) return EXIT_FAILURE; #define DO_TEST(arch, name) \ - data.archName = arch; \ - data.base = name; \ - if (virTestRun(name "(" arch ")", testQemuCapsXML, &data) < 0) \ - data.ret = -1 + do { \ + VIR_AUTOFREE(char *) title = NULL; \ + if (virAsprintf(&title, "%s (%s)", name, arch) < 0) \ + return -EXIT_FAILURE; \ + data.archName = arch; \ + data.base = name; \ + if (virTestRun(title, testQemuCapsXML, &data) < 0) \ + data.ret = -1; \ + } while (0) /* Keep this in sync with qemucapabilitiestest */ DO_TEST("x86_64", "caps_1.5.3"); -- 2.20.1
On Thu, Mar 07, 2019 at 16:44:33 +0100, Andrea Bolognani wrote:
We're using static string concatenation at the moment, but that will no longer be a possibility in a bit.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 11 ++++++++--- tests/qemucaps2xmltest.c | 13 +++++++++---- 2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index e3c6681dd4..222ac05d79 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -196,12 +196,17 @@ mymain(void)
#define DO_TEST(arch, name) \ do { \ + VIR_AUTOFREE(char *) title = NULL; \ + VIR_AUTOFREE(char *) copyTitle = NULL; \ + if (virAsprintf(&title, "%s (%s)", name, arch) < 0 || \ + virAsprintf(©Title, "copy %s (%s)", name, arch) < 0) { \ + return -EXIT_FAILURE; \
Coding style. Single-line body.
+ } \ data.archName = arch; \ data.base = name; \ - if (virTestRun(name "(" arch ")", testQemuCaps, &data) < 0) \ + if (virTestRun(title, testQemuCaps, &data) < 0) \ data.ret = -1; \ - if (virTestRun("copy " name "(" arch ")", \ - testQemuCapsCopy, &data) < 0) \ + if (virTestRun(copyTitle, testQemuCapsCopy, &data) < 0) \ data.ret = -1; \ } while (0)
ACK
On Wed, 2019-03-13 at 09:48 +0100, Peter Krempa wrote:
On Thu, Mar 07, 2019 at 16:44:33 +0100, Andrea Bolognani wrote: [...]
#define DO_TEST(arch, name) \ do { \ + VIR_AUTOFREE(char *) title = NULL; \ + VIR_AUTOFREE(char *) copyTitle = NULL; \ + if (virAsprintf(&title, "%s (%s)", name, arch) < 0 || \ + virAsprintf(©Title, "copy %s (%s)", name, arch) < 0) { \ + return -EXIT_FAILURE; \
Coding style. Single-line body.
There are multiple conditions with the same indentation, so per the coding guidelines[1] the curly braces are required. Honesly, we should really give clang-format or whatever similar tool a serious go and just start enforcing that code needs to be filtered through it before being merged. Having humans worry about this kind of nonsense is such an utter waste of time. [1] https://libvirt.org/hacking.html#curly_braces, third example. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 13, 2019 at 10:25:16 +0100, Andrea Bolognani wrote:
On Wed, 2019-03-13 at 09:48 +0100, Peter Krempa wrote:
On Thu, Mar 07, 2019 at 16:44:33 +0100, Andrea Bolognani wrote: [...]
#define DO_TEST(arch, name) \ do { \ + VIR_AUTOFREE(char *) title = NULL; \ + VIR_AUTOFREE(char *) copyTitle = NULL; \ + if (virAsprintf(&title, "%s (%s)", name, arch) < 0 || \ + virAsprintf(©Title, "copy %s (%s)", name, arch) < 0) { \ + return -EXIT_FAILURE; \
Coding style. Single-line body.
There are multiple conditions with the same indentation, so per the coding guidelines[1] the curly braces are required.
Honesly, we should really give clang-format or whatever similar tool a serious go and just start enforcing that code needs to be filtered through it before being merged. Having humans worry about this kind of nonsense is such an utter waste of time.
[1] https://libvirt.org/hacking.html#curly_braces, third example.
Hmm, interresting. In this particular instance we are pretty much always breaking the style though. Majority of multi-line conditions with a single line body which I've encountered don't have the block. ACK as is.
On Wed, 2019-03-13 at 10:32 +0100, Peter Krempa wrote:
On Wed, Mar 13, 2019 at 10:25:16 +0100, Andrea Bolognani wrote:
On Wed, 2019-03-13 at 09:48 +0100, Peter Krempa wrote:
On Thu, Mar 07, 2019 at 16:44:33 +0100, Andrea Bolognani wrote: [...]
#define DO_TEST(arch, name) \ do { \ + VIR_AUTOFREE(char *) title = NULL; \ + VIR_AUTOFREE(char *) copyTitle = NULL; \ + if (virAsprintf(&title, "%s (%s)", name, arch) < 0 || \ + virAsprintf(©Title, "copy %s (%s)", name, arch) < 0) { \ + return -EXIT_FAILURE; \
Coding style. Single-line body.
There are multiple conditions with the same indentation, so per the coding guidelines[1] the curly braces are required.
Honesly, we should really give clang-format or whatever similar tool a serious go and just start enforcing that code needs to be filtered through it before being merged. Having humans worry about this kind of nonsense is such an utter waste of time.
[1] https://libvirt.org/hacking.html#curly_braces, third example.
Hmm, interresting. In this particular instance we are pretty much always breaking the style though. Majority of multi-line conditions with a single line body which I've encountered don't have the block.
Yeah, none of the style rules that doesn't have a corresponding syntax-check rule is really enforced consistently, which is why we should consider flipping the workflow and just have a tool reformat the code for us in the first place. -- Andrea Bolognani / Red Hat / Virtualization
This removes the awkard escaping and will allow us to perform some interesting refactoring later on. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 40 +++++++++++++++++++++++++----------- tests/qemucaps2xmltest.c | 28 ++++++++++++++++++------- 2 files changed, 49 insertions(+), 19 deletions(-) diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index 222ac05d79..b4ed081d3e 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -176,6 +176,32 @@ testQemuCapsCopy(const void *opaque) } +static int +doCapsTest(const char *base, + const char *archName, + testQemuDataPtr data) +{ + VIR_AUTOFREE(char *) title = NULL; + VIR_AUTOFREE(char *) copyTitle = NULL; + + if (virAsprintf(&title, "%s (%s)", base, archName) < 0 || + virAsprintf(©Title, "copy %s (%s)", base, archName) < 0) { + return -1; + } + + data->base = base; + data->archName = archName; + + if (virTestRun(title, testQemuCaps, data) < 0) + data->ret = -1; + + if (virTestRun(copyTitle, testQemuCapsCopy, data) < 0) + data->ret = -1; + + return 0; +} + + static int mymain(void) { @@ -196,18 +222,8 @@ mymain(void) #define DO_TEST(arch, name) \ do { \ - VIR_AUTOFREE(char *) title = NULL; \ - VIR_AUTOFREE(char *) copyTitle = NULL; \ - if (virAsprintf(&title, "%s (%s)", name, arch) < 0 || \ - virAsprintf(©Title, "copy %s (%s)", name, arch) < 0) { \ - return -EXIT_FAILURE; \ - } \ - data.archName = arch; \ - data.base = name; \ - if (virTestRun(title, testQemuCaps, &data) < 0) \ - data.ret = -1; \ - if (virTestRun(copyTitle, testQemuCapsCopy, &data) < 0) \ - data.ret = -1; \ + if (doCapsTest(name, arch, &data) < 0) \ + return EXIT_FAILURE; \ } while (0) /* Keep this in sync with qemucaps2xmltest */ diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index be460b42f8..4f9cfc459e 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -178,6 +178,25 @@ testQemuCapsXML(const void *opaque) return ret; } +static int +doCapsTest(const char *base, + const char *archName, + testQemuDataPtr data) +{ + VIR_AUTOFREE(char *) title = NULL; + + if (virAsprintf(&title, "%s (%s)", base, archName) < 0) + return -1; + + data->base = base; + data->archName = archName; + + if (virTestRun(title, testQemuCapsXML, data) < 0) + data->ret = -1; + + return 0; +} + static int mymain(void) { @@ -198,13 +217,8 @@ mymain(void) #define DO_TEST(arch, name) \ do { \ - VIR_AUTOFREE(char *) title = NULL; \ - if (virAsprintf(&title, "%s (%s)", name, arch) < 0) \ - return -EXIT_FAILURE; \ - data.archName = arch; \ - data.base = name; \ - if (virTestRun(title, testQemuCapsXML, &data) < 0) \ - data.ret = -1; \ + if (doCapsTest(name, arch, &data) < 0) \ + return EXIT_FAILURE; \ } while (0) /* Keep this in sync with qemucapabilitiestest */ -- 2.20.1
On Thu, Mar 07, 2019 at 16:44:34 +0100, Andrea Bolognani wrote:
This removes the awkard escaping and will allow us to perform some interesting refactoring later on.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 40 +++++++++++++++++++++++++----------- tests/qemucaps2xmltest.c | 28 ++++++++++++++++++------- 2 files changed, 49 insertions(+), 19 deletions(-)
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index 222ac05d79..b4ed081d3e 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -176,6 +176,32 @@ testQemuCapsCopy(const void *opaque) }
+static int +doCapsTest(const char *base, + const char *archName, + testQemuDataPtr data) +{ + VIR_AUTOFREE(char *) title = NULL; + VIR_AUTOFREE(char *) copyTitle = NULL; + + if (virAsprintf(&title, "%s (%s)", base, archName) < 0 || + virAsprintf(©Title, "copy %s (%s)", base, archName) < 0) { + return -1; + }
Single line body.
+ + data->base = base; + data->archName = archName; + + if (virTestRun(title, testQemuCaps, data) < 0) + data->ret = -1; + + if (virTestRun(copyTitle, testQemuCapsCopy, data) < 0) + data->ret = -1; + + return 0; +}
ACK
We're not using any of the functionality offered by the module at the moment, but we will in just a second. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/Makefile.am | 1 + tests/qemucaps2xmltest.c | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index 72f0420bab..46fa50cc0a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -639,6 +639,7 @@ qemucommandutiltest_LDADD = libqemumonitortestutils.la \ qemucaps2xmltest_SOURCES = \ qemucaps2xmltest.c \ testutils.c testutils.h \ + testutilsqemu.c testutilsqemu.h \ $(NULL) qemucaps2xmltest_LDADD = $(qemu_LDADDS) $(LDADDS) diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index 4f9cfc459e..dff4e2a884 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -19,6 +19,7 @@ #include <config.h> #include "testutils.h" +#include "testutilsqemu.h" #include "qemu/qemu_capabilities.h" -- 2.20.1
On Thu, Mar 07, 2019 at 16:44:35 +0100, Andrea Bolognani wrote:
We're not using any of the functionality offered by the module at the moment, but we will in just a second.
I'd squash this to the patch actually using something from the module. ACK regardless.
This function iterates over a directory containing capabilities-related data, extract some useful bits of information from the file name, and calls a user-provided callback. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/testutilsqemu.c | 53 +++++++++++++++++++++++++++++++++++++++++++ tests/testutilsqemu.h | 8 +++++++ 2 files changed, 61 insertions(+) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 61bf67d5ad..407cf91925 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -865,3 +865,56 @@ testQemuGetLatestCapsForArch(const char *dirname, virDirClose(&dir); return ret; } + + +int +testQemuCapsIterate(const char *dirname, + const char *suffix, + testQemuCapsIterateCallback callback, + void *opaque) +{ + struct dirent *ent; + DIR *dir = NULL; + int rc; + int ret = -1; + + if (!callback) + return 0; + + if (virDirOpen(&dir, dirname) < 0) + goto cleanup; + + while ((rc = virDirRead(dir, &ent, dirname) > 0)) { + char *tmp = ent->d_name; + char *base = NULL; + char *archName = NULL; + + /* Strip the trailing suffix, moving on if it's not present */ + if (!virStringStripSuffix(tmp, suffix)) + continue; + + /* Find the last dot, moving on if none is present */ + if (!(archName = strrchr(tmp, '.'))) + continue; + + /* The base name is everything before the last dot, and + * the architecture name everything after it */ + base = tmp; + archName[0] = '\0'; + archName++; + + /* Run the user-provided callback */ + if (callback(base, archName, opaque) < 0) + goto cleanup; + } + + if (rc < 0) + goto cleanup; + + ret = 0; + + cleanup: + virDirClose(&dir); + + return ret; +} diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index 5ae7f19473..183ce915f1 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -63,6 +63,14 @@ char *testQemuGetLatestCapsForArch(const char *dirname, const char *arch, const char *suffix); +typedef int (*testQemuCapsIterateCallback)(const char *base, + const char *archName, + void *opaque); +int testQemuCapsIterate(const char *dirname, + const char *suffix, + testQemuCapsIterateCallback callback, + void *opaque); + # endif #endif /* LIBVIRT_TESTUTILSQEMU_H */ -- 2.20.1
On Thu, Mar 07, 2019 at 16:44:36 +0100, Andrea Bolognani wrote:
This function iterates over a directory containing capabilities-related data, extract some useful bits of information from the file name, and calls a user-provided callback.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/testutilsqemu.c | 53 +++++++++++++++++++++++++++++++++++++++++++ tests/testutilsqemu.h | 8 +++++++ 2 files changed, 61 insertions(+)
ACK
With only a couple minor tweaks, we can make the existing doCapsTest() functions with testQemuCapsIterate() and finally remove the need to manually adjust the test programs every time a new input file is introduced; moreover, this means that the two lists can't possibly get out of sync anymore. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 48 +++--------------------------------- tests/qemucaps2xmltest.c | 48 +++--------------------------------- 2 files changed, 8 insertions(+), 88 deletions(-) diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index b4ed081d3e..16c2832ffb 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -179,8 +179,9 @@ testQemuCapsCopy(const void *opaque) static int doCapsTest(const char *base, const char *archName, - testQemuDataPtr data) + void *opaque) { + testQemuDataPtr data = (testQemuDataPtr) opaque; VIR_AUTOFREE(char *) title = NULL; VIR_AUTOFREE(char *) copyTitle = NULL; @@ -220,49 +221,8 @@ mymain(void) if (testQemuDataInit(&data) < 0) return EXIT_FAILURE; -#define DO_TEST(arch, name) \ - do { \ - if (doCapsTest(name, arch, &data) < 0) \ - return EXIT_FAILURE; \ - } while (0) - - /* Keep this in sync with qemucaps2xmltest */ - DO_TEST("x86_64", "caps_1.5.3"); - DO_TEST("x86_64", "caps_1.6.0"); - DO_TEST("x86_64", "caps_1.7.0"); - DO_TEST("x86_64", "caps_2.1.1"); - DO_TEST("x86_64", "caps_2.4.0"); - DO_TEST("x86_64", "caps_2.5.0"); - DO_TEST("x86_64", "caps_2.6.0"); - DO_TEST("x86_64", "caps_2.7.0"); - DO_TEST("x86_64", "caps_2.8.0"); - DO_TEST("x86_64", "caps_2.9.0"); - DO_TEST("x86_64", "caps_2.10.0"); - DO_TEST("x86_64", "caps_2.11.0"); - DO_TEST("x86_64", "caps_2.12.0"); - DO_TEST("x86_64", "caps_3.0.0"); - DO_TEST("x86_64", "caps_3.1.0"); - DO_TEST("x86_64", "caps_4.0.0"); - DO_TEST("aarch64", "caps_2.6.0"); - DO_TEST("aarch64", "caps_2.10.0"); - DO_TEST("aarch64", "caps_2.12.0"); - DO_TEST("ppc64", "caps_2.6.0"); - DO_TEST("ppc64", "caps_2.9.0"); - DO_TEST("ppc64", "caps_2.10.0"); - DO_TEST("ppc64", "caps_2.12.0"); - DO_TEST("ppc64", "caps_3.0.0"); - DO_TEST("ppc64", "caps_3.1.0"); - DO_TEST("s390x", "caps_2.7.0"); - DO_TEST("s390x", "caps_2.8.0"); - DO_TEST("s390x", "caps_2.9.0"); - DO_TEST("s390x", "caps_2.10.0"); - DO_TEST("s390x", "caps_2.11.0"); - DO_TEST("s390x", "caps_2.12.0"); - DO_TEST("s390x", "caps_3.0.0"); - DO_TEST("riscv32", "caps_3.0.0"); - DO_TEST("riscv32", "caps_4.0.0"); - DO_TEST("riscv64", "caps_3.0.0"); - DO_TEST("riscv64", "caps_4.0.0"); + if (testQemuCapsIterate(data.dataDir, ".replies", doCapsTest, &data) < 0) + return EXIT_FAILURE; /* * Run "tests/qemucapsprobe /path/to/qemu/binary >foo.replies" diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index dff4e2a884..e21fde7e0b 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -182,8 +182,9 @@ testQemuCapsXML(const void *opaque) static int doCapsTest(const char *base, const char *archName, - testQemuDataPtr data) + void *opaque) { + testQemuDataPtr data = (testQemuDataPtr) opaque; VIR_AUTOFREE(char *) title = NULL; if (virAsprintf(&title, "%s (%s)", base, archName) < 0) @@ -216,49 +217,8 @@ mymain(void) if (testQemuDataInit(&data) < 0) return EXIT_FAILURE; -#define DO_TEST(arch, name) \ - do { \ - if (doCapsTest(name, arch, &data) < 0) \ - return EXIT_FAILURE; \ - } while (0) - - /* Keep this in sync with qemucapabilitiestest */ - DO_TEST("x86_64", "caps_1.5.3"); - DO_TEST("x86_64", "caps_1.6.0"); - DO_TEST("x86_64", "caps_1.7.0"); - DO_TEST("x86_64", "caps_2.1.1"); - DO_TEST("x86_64", "caps_2.4.0"); - DO_TEST("x86_64", "caps_2.5.0"); - DO_TEST("x86_64", "caps_2.6.0"); - DO_TEST("x86_64", "caps_2.7.0"); - DO_TEST("x86_64", "caps_2.8.0"); - DO_TEST("x86_64", "caps_2.9.0"); - DO_TEST("x86_64", "caps_2.10.0"); - DO_TEST("x86_64", "caps_2.11.0"); - DO_TEST("x86_64", "caps_2.12.0"); - DO_TEST("x86_64", "caps_3.0.0"); - DO_TEST("x86_64", "caps_3.1.0"); - DO_TEST("x86_64", "caps_4.0.0"); - DO_TEST("aarch64", "caps_2.6.0"); - DO_TEST("aarch64", "caps_2.10.0"); - DO_TEST("aarch64", "caps_2.12.0"); - DO_TEST("ppc64", "caps_2.6.0"); - DO_TEST("ppc64", "caps_2.9.0"); - DO_TEST("ppc64", "caps_2.10.0"); - DO_TEST("ppc64", "caps_2.12.0"); - DO_TEST("ppc64", "caps_3.0.0"); - DO_TEST("ppc64", "caps_3.1.0"); - DO_TEST("s390x", "caps_2.7.0"); - DO_TEST("s390x", "caps_2.8.0"); - DO_TEST("s390x", "caps_2.9.0"); - DO_TEST("s390x", "caps_2.10.0"); - DO_TEST("s390x", "caps_2.11.0"); - DO_TEST("s390x", "caps_2.12.0"); - DO_TEST("s390x", "caps_3.0.0"); - DO_TEST("riscv32", "caps_3.0.0"); - DO_TEST("riscv32", "caps_4.0.0"); - DO_TEST("riscv64", "caps_3.0.0"); - DO_TEST("riscv64", "caps_4.0.0"); + if (testQemuCapsIterate(data.inputDir, ".xml", doCapsTest, &data) < 0) + return EXIT_FAILURE; testQemuDataReset(&data); -- 2.20.1
On Thu, Mar 07, 2019 at 16:44:37 +0100, Andrea Bolognani wrote:
With only a couple minor tweaks, we can make the existing doCapsTest() functions with testQemuCapsIterate() and finally remove the need to manually adjust the test programs every time a new input file is introduced; moreover, this means that the two lists can't possibly get out of sync anymore.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiestest.c | 48 +++--------------------------------- tests/qemucaps2xmltest.c | 48 +++--------------------------------- 2 files changed, 8 insertions(+), 88 deletions(-)
ACK
participants (2)
-
Andrea Bolognani -
Peter Krempa