[PATCH v2 0/3] Implement virDomainGetMessages for test driver

Implement virDomainGetMessages API for test driver. v2: Introduce testDomainObjCheckTaint to test the API. Luke Yue (3): test_driver: Implement virDomainGetMessages test_driver: Introduce testDomainObjCheckTaint tests: Add messages for virshtest src/test/test_driver.c | 110 +++++++++++++++++++++++++++++++++++++++++ tests/virshtest.c | 2 + 2 files changed, 112 insertions(+) -- 2.32.0

Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 65710b78ef..dff96bceb6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9331,6 +9331,58 @@ testDomainCheckpointDelete(virDomainCheckpointPtr checkpoint, return ret; } +static int +testDomainGetMessages(virDomainPtr dom, + char ***msgs, + unsigned int flags) +{ + virDomainObj *vm = NULL; + int rv = -1; + size_t i, n; + int nmsgs; + + virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION | + VIR_DOMAIN_MESSAGE_TAINTING, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + *msgs = NULL; + nmsgs = 0; + n = 0; + + if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) { + nmsgs += __builtin_popcount(vm->taint); + *msgs = g_renew(char *, *msgs, nmsgs+1); + + for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) { + if (vm->taint & (1 << i)) { + (*msgs)[n++] = g_strdup_printf( + _("tainted: %s"), + _(virDomainTaintMessageTypeToString(i))); + } + } + } + + if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) { + nmsgs += vm->ndeprecations; + *msgs = g_renew(char *, *msgs, nmsgs+1); + + for (i = 0; i < vm->ndeprecations; i++) { + (*msgs)[n++] = g_strdup_printf( + _("deprecated configuration: %s"), + vm->deprecations[i]); + } + } + + (*msgs)[nmsgs] = NULL; + + rv = nmsgs; + + virDomainObjEndAPI(&vm); + return rv; +} + /* * Test driver */ @@ -9489,6 +9541,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainCheckpointLookupByName = testDomainCheckpointLookupByName, /* 5.6.0 */ .domainCheckpointGetParent = testDomainCheckpointGetParent, /* 5.6.0 */ .domainCheckpointDelete = testDomainCheckpointDelete, /* 5.6.0 */ + .domainGetMessages = testDomainGetMessages, /* 7.5.0 */ }; static virNetworkDriver testNetworkDriver = { -- 2.32.0

On Thu, Jun 24, 2021 at 06:59:59PM +0800, Luke Yue wrote:
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 65710b78ef..dff96bceb6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9331,6 +9331,58 @@ testDomainCheckpointDelete(virDomainCheckpointPtr checkpoint, return ret; }
+static int +testDomainGetMessages(virDomainPtr dom, + char ***msgs, + unsigned int flags) +{ + virDomainObj *vm = NULL; + int rv = -1; + size_t i, n; + int nmsgs; + + virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION | + VIR_DOMAIN_MESSAGE_TAINTING, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + *msgs = NULL; + nmsgs = 0; + n = 0; + + if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) { + nmsgs += __builtin_popcount(vm->taint); + *msgs = g_renew(char *, *msgs, nmsgs+1); + + for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) { + if (vm->taint & (1 << i)) { + (*msgs)[n++] = g_strdup_printf( + _("tainted: %s"), + _(virDomainTaintMessageTypeToString(i))); + } + } + } + + if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) { + nmsgs += vm->ndeprecations; + *msgs = g_renew(char *, *msgs, nmsgs+1); + + for (i = 0; i < vm->ndeprecations; i++) { + (*msgs)[n++] = g_strdup_printf( + _("deprecated configuration: %s"), + vm->deprecations[i]); + } + } + + (*msgs)[nmsgs] = NULL; + + rv = nmsgs; + + virDomainObjEndAPI(&vm); + return rv; +} +
As I mentioned in the review for v1, it is nice that someone can check how this API behaves without spinning up a VM, just using the test driver. However most of this code is copy-paste from the qemu driver and can hence be extracted to a separate function which would be called from both drivers (and maybe more than the ones mentioned here are using the same logic). That would make the codebase smaller, the library smaller, it would keep the test driver updated and it would actually test the codepath used in the qemu driver.
/* * Test driver */ @@ -9489,6 +9541,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainCheckpointLookupByName = testDomainCheckpointLookupByName, /* 5.6.0 */ .domainCheckpointGetParent = testDomainCheckpointGetParent, /* 5.6.0 */ .domainCheckpointDelete = testDomainCheckpointDelete, /* 5.6.0 */ + .domainGetMessages = testDomainGetMessages, /* 7.5.0 */ };
static virNetworkDriver testNetworkDriver = { -- 2.32.0

On Thu, 2021-06-24 at 17:19 +0200, Martin Kletzander wrote:
On Thu, Jun 24, 2021 at 06:59:59PM +0800, Luke Yue wrote:
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 65710b78ef..dff96bceb6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9331,6 +9331,58 @@ testDomainCheckpointDelete(virDomainCheckpointPtr checkpoint, return ret; }
+static int +testDomainGetMessages(virDomainPtr dom, + char ***msgs, + unsigned int flags) +{ + virDomainObj *vm = NULL; + int rv = -1; + size_t i, n; + int nmsgs; + + virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION | + VIR_DOMAIN_MESSAGE_TAINTING, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + *msgs = NULL; + nmsgs = 0; + n = 0; + + if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) { + nmsgs += __builtin_popcount(vm->taint); + *msgs = g_renew(char *, *msgs, nmsgs+1); + + for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) { + if (vm->taint & (1 << i)) { + (*msgs)[n++] = g_strdup_printf( + _("tainted: %s"), + _(virDomainTaintMessageTypeToString(i))); + } + } + } + + if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) { + nmsgs += vm->ndeprecations; + *msgs = g_renew(char *, *msgs, nmsgs+1); + + for (i = 0; i < vm->ndeprecations; i++) { + (*msgs)[n++] = g_strdup_printf( + _("deprecated configuration: %s"), + vm->deprecations[i]); + } + } + + (*msgs)[nmsgs] = NULL; + + rv = nmsgs; + + virDomainObjEndAPI(&vm); + return rv; +} +
As I mentioned in the review for v1, it is nice that someone can check how this API behaves without spinning up a VM, just using the test driver. However most of this code is copy-paste from the qemu driver and can hence be extracted to a separate function which would be called from both drivers (and maybe more than the ones mentioned here are using the same logic). That would make the codebase smaller, the library smaller, it would keep the test driver updated and it would actually test the codepath used in the qemu driver.
I'm sorry, I forgot that. So if I want to extract a function like below ``` int virDomainGetMessagesFromVM(virDomainObj *vm, char **msgs, unsigned int flags) { size_t i, n; int nmsgs; int rv = -1; *msgs = NULL; nmsgs = 0; n = 0; ... rv = nmsgs; return rv; } ``` Where should I place this function? In `domain_conf.c` or somewhere else? Thanks, Luke

On Mon, Jun 28, 2021 at 10:45:38AM +0800, Luke Yue wrote:
On Thu, 2021-06-24 at 17:19 +0200, Martin Kletzander wrote:
On Thu, Jun 24, 2021 at 06:59:59PM +0800, Luke Yue wrote:
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 65710b78ef..dff96bceb6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9331,6 +9331,58 @@ testDomainCheckpointDelete(virDomainCheckpointPtr checkpoint, return ret; }
+static int +testDomainGetMessages(virDomainPtr dom, + char ***msgs, + unsigned int flags) +{ + virDomainObj *vm = NULL; + int rv = -1; + size_t i, n; + int nmsgs; + + virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION | + VIR_DOMAIN_MESSAGE_TAINTING, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + *msgs = NULL; + nmsgs = 0; + n = 0; + + if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) { + nmsgs += __builtin_popcount(vm->taint); + *msgs = g_renew(char *, *msgs, nmsgs+1); + + for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) { + if (vm->taint & (1 << i)) { + (*msgs)[n++] = g_strdup_printf( + _("tainted: %s"), + _(virDomainTaintMessageTypeToString(i))); + } + } + } + + if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) { + nmsgs += vm->ndeprecations; + *msgs = g_renew(char *, *msgs, nmsgs+1); + + for (i = 0; i < vm->ndeprecations; i++) { + (*msgs)[n++] = g_strdup_printf( + _("deprecated configuration: %s"), + vm->deprecations[i]); + } + } + + (*msgs)[nmsgs] = NULL; + + rv = nmsgs; + + virDomainObjEndAPI(&vm); + return rv; +} +
As I mentioned in the review for v1, it is nice that someone can check how this API behaves without spinning up a VM, just using the test driver. However most of this code is copy-paste from the qemu driver and can hence be extracted to a separate function which would be called from both drivers (and maybe more than the ones mentioned here are using the same logic). That would make the codebase smaller, the library smaller, it would keep the test driver updated and it would actually test the codepath used in the qemu driver.
I'm sorry, I forgot that. So if I want to extract a function like below
``` int virDomainGetMessagesFromVM(virDomainObj *vm, char **msgs, unsigned int flags) { size_t i, n; int nmsgs; int rv = -1;
*msgs = NULL; nmsgs = 0; n = 0;
...
rv = nmsgs;
return rv; } ```
Where should I place this function? In `domain_conf.c` or somewhere else?
Good question. We do not have that many cases of such functions, so it does not have one file in the repo, however you can start by looking at what are the main parameters of the function. This one operates on virDomainObj, so it can either be in the driver itself (which we are trying to avoid) and somewhere in src/conf based on further info. Searching for other such functions, I just used a simple grep for this: rg '\(virDomainObj \*' src/conf or: git grep '(virDomainObj \*' src/conf and you can see that it does not fit in domain_event or domain_audit and it will most probably just be in domain.conf just like you suggested. I must admit that it is not a very clean approach as src/conf is mostly handling virDomainDef, but I guess we do not have enough of similar functions that would facilitate another creation of a new file in the directory. I would not stress myself out too much about that as the worst thing that can happen is that someone will have a better idea.
Thanks, Luke

On Mon, 2021-06-28 at 16:12 +0200, Martin Kletzander wrote:
On Mon, Jun 28, 2021 at 10:45:38AM +0800, Luke Yue wrote:
On Thu, 2021-06-24 at 17:19 +0200, Martin Kletzander wrote:
On Thu, Jun 24, 2021 at 06:59:59PM +0800, Luke Yue wrote:
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 65710b78ef..dff96bceb6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9331,6 +9331,58 @@ testDomainCheckpointDelete(virDomainCheckpointPtr checkpoint, return ret; }
+static int +testDomainGetMessages(virDomainPtr dom, + char ***msgs, + unsigned int flags) +{ + virDomainObj *vm = NULL; + int rv = -1; + size_t i, n; + int nmsgs; + + virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION | + VIR_DOMAIN_MESSAGE_TAINTING, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + *msgs = NULL; + nmsgs = 0; + n = 0; + + if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) { + nmsgs += __builtin_popcount(vm->taint); + *msgs = g_renew(char *, *msgs, nmsgs+1); + + for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) { + if (vm->taint & (1 << i)) { + (*msgs)[n++] = g_strdup_printf( + _("tainted: %s"), + _(virDomainTaintMessageTypeToString(i))); + } + } + } + + if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) { + nmsgs += vm->ndeprecations; + *msgs = g_renew(char *, *msgs, nmsgs+1); + + for (i = 0; i < vm->ndeprecations; i++) { + (*msgs)[n++] = g_strdup_printf( + _("deprecated configuration: %s"), + vm->deprecations[i]); + } + } + + (*msgs)[nmsgs] = NULL; + + rv = nmsgs; + + virDomainObjEndAPI(&vm); + return rv; +} +
As I mentioned in the review for v1, it is nice that someone can check how this API behaves without spinning up a VM, just using the test driver. However most of this code is copy-paste from the qemu driver and can hence be extracted to a separate function which would be called from both drivers (and maybe more than the ones mentioned here are using the same logic). That would make the codebase smaller, the library smaller, it would keep the test driver updated and it would actually test the codepath used in the qemu driver.
I'm sorry, I forgot that. So if I want to extract a function like below
``` int virDomainGetMessagesFromVM(virDomainObj *vm, char **msgs, unsigned int flags) { size_t i, n; int nmsgs; int rv = -1;
*msgs = NULL; nmsgs = 0; n = 0;
...
rv = nmsgs;
return rv; } ```
Where should I place this function? In `domain_conf.c` or somewhere else?
Good question. We do not have that many cases of such functions, so it does not have one file in the repo, however you can start by looking at what are the main parameters of the function. This one operates on virDomainObj, so it can either be in the driver itself (which we are trying to avoid) and somewhere in src/conf based on further info. Searching for other such functions, I just used a simple grep for this:
rg '\(virDomainObj \*' src/conf
or:
git grep '(virDomainObj \*' src/conf
and you can see that it does not fit in domain_event or domain_audit and it will most probably just be in domain.conf just like you suggested. I must admit that it is not a very clean approach as src/conf is mostly handling virDomainDef, but I guess we do not have enough of similar functions that would facilitate another creation of a new file in the directory. I would not stress myself out too much about that as the worst thing that can happen is that someone will have a better idea.
Thanks so much! I learned a lot from you. So I will implement the function and put it in domain_conf.c until someone gives a better solution. And I have another question, as it's freeze for 7.5.0, should I change the version number in comments to 7.6.0 in the new patches? Thanks!

On Mon, Jun 28, 2021 at 11:36:45PM +0800, Luke Yue wrote:
And I have another question, as it's freeze for 7.5.0, should I change the version number in comments to 7.6.0 in the new patches? Thanks!
Yep, it won't make 7.5.0 now. Thanks.

In order to test the virDomainGetMessages for test driver, we need to check some taints or deprecations, so introduce testDomainObjCheckTaint for checking taints. Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 57 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index dff96bceb6..88ae11c448 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9331,6 +9331,61 @@ testDomainCheckpointDelete(virDomainCheckpointPtr checkpoint, return ret; } +static void +testDomainObjCheckDiskTaint(virDomainObj *obj, + virDomainDiskDef *disk) +{ + if (disk->rawio == VIR_TRISTATE_BOOL_YES) + virDomainObjTaint(obj, VIR_DOMAIN_TAINT_HIGH_PRIVILEGES); + + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM && + virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK && + disk->src->path && virFileIsCDROM(disk->src->path) == 1) + virDomainObjTaint(obj, VIR_DOMAIN_TAINT_CDROM_PASSTHROUGH); +} + +static void +testDomainObjCheckHostdevTaint(virDomainObj *obj, + virDomainHostdevDef *hostdev) +{ + if (!virHostdevIsSCSIDevice(hostdev)) + return; + + if (hostdev->source.subsys.u.scsi.rawio == VIR_TRISTATE_BOOL_YES) + virDomainObjTaint(obj, VIR_DOMAIN_TAINT_HIGH_PRIVILEGES); +} + +static void +testDomainObjCheckNetTaint(virDomainObj *obj, + virDomainNetDef *net) +{ + /* script is only useful for NET_TYPE_ETHERNET (qemu) and + * NET_TYPE_BRIDGE (xen), but could be (incorrectly) specified for + * any interface type. In any case, it's adding user sauce into + * the soup, so it should taint the domain. + */ + if (net->script != NULL) + virDomainObjTaint(obj, VIR_DOMAIN_TAINT_SHELL_SCRIPTS); +} + +static void +testDomainObjCheckTaint(virDomainObj *obj) +{ + size_t i; + + for (i = 0; i < obj->def->ndisks; i++) + testDomainObjCheckDiskTaint(obj, obj->def->disks[i]); + + for (i = 0; i < obj->def->nhostdevs; i++) + testDomainObjCheckHostdevTaint(obj, obj->def->hostdevs[i]); + + for (i = 0; i < obj->def->nnets; i++) + testDomainObjCheckNetTaint(obj, obj->def->nets[i]); + + if (obj->def->os.dtb) + virDomainObjTaint(obj, VIR_DOMAIN_TAINT_CUSTOM_DTB); +} + static int testDomainGetMessages(virDomainPtr dom, char ***msgs, @@ -9351,6 +9406,8 @@ testDomainGetMessages(virDomainPtr dom, nmsgs = 0; n = 0; + testDomainObjCheckTaint(vm); + if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) { nmsgs += __builtin_popcount(vm->taint); *msgs = g_renew(char *, *msgs, nmsgs+1); -- 2.32.0

As we introduced testDomainObjCheckTaint for test driver, the `dominfo` command in virshtest will now print tainting messages, so add them for test. Signed-off-by: Luke Yue <lukedyue@gmail.com> --- tests/virshtest.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/virshtest.c b/tests/virshtest.c index c1974c46cb..937448cefc 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -22,6 +22,7 @@ main(void) # define DOM_UUID "ef861801-45b9-11cb-88e3-afbfe5370493" # define SECURITY_LABEL "libvirt-test (enforcing)" +# define MESSAGES "tainted: network configuration using opaque shell scripts" static const char *dominfo_fc4 = "\ Id: 2\n\ @@ -38,6 +39,7 @@ Managed save: no\n\ Security model: testSecurity\n\ Security DOI: \n\ Security label: " SECURITY_LABEL "\n\ +Messages: " MESSAGES "\n\ \n"; static const char *domuuid_fc4 = DOM_UUID "\n\n"; static const char *domid_fc4 = "2\n\n"; -- 2.32.0

On Thu, Jun 24, 2021 at 07:00:01PM +0800, Luke Yue wrote:
As we introduced testDomainObjCheckTaint for test driver, the `dominfo` command in virshtest will now print tainting messages, so add them for test.
We are trying to keep the code pass the test after each commit (easier for bisection etc.) so this change should be squashed into the commit that introduced the change which caused the output to change.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- tests/virshtest.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tests/virshtest.c b/tests/virshtest.c index c1974c46cb..937448cefc 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -22,6 +22,7 @@ main(void)
# define DOM_UUID "ef861801-45b9-11cb-88e3-afbfe5370493" # define SECURITY_LABEL "libvirt-test (enforcing)" +# define MESSAGES "tainted: network configuration using opaque shell scripts"
static const char *dominfo_fc4 = "\ Id: 2\n\ @@ -38,6 +39,7 @@ Managed save: no\n\ Security model: testSecurity\n\ Security DOI: \n\ Security label: " SECURITY_LABEL "\n\ +Messages: " MESSAGES "\n\ \n"; static const char *domuuid_fc4 = DOM_UUID "\n\n"; static const char *domid_fc4 = "2\n\n"; -- 2.32.0

On Thu, 2021-06-24 at 17:14 +0200, Martin Kletzander wrote:
On Thu, Jun 24, 2021 at 07:00:01PM +0800, Luke Yue wrote:
As we introduced testDomainObjCheckTaint for test driver, the `dominfo` command in virshtest will now print tainting messages, so add them for test.
We are trying to keep the code pass the test after each commit (easier for bisection etc.) so this change should be squashed into the commit that introduced the change which caused the output to change.
OK, I will squash this commit in v3, thanks!
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- tests/virshtest.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tests/virshtest.c b/tests/virshtest.c index c1974c46cb..937448cefc 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -22,6 +22,7 @@ main(void)
# define DOM_UUID "ef861801-45b9-11cb-88e3-afbfe5370493" # define SECURITY_LABEL "libvirt-test (enforcing)" +# define MESSAGES "tainted: network configuration using opaque shell scripts"
static const char *dominfo_fc4 = "\ Id: 2\n\ @@ -38,6 +39,7 @@ Managed save: no\n\ Security model: testSecurity\n\ Security DOI: \n\ Security label: " SECURITY_LABEL "\n\ +Messages: " MESSAGES "\n\ \n"; static const char *domuuid_fc4 = DOM_UUID "\n\n"; static const char *domid_fc4 = "2\n\n"; -- 2.32.0
participants (2)
-
Luke Yue
-
Martin Kletzander