On 6/17/21 5:55 AM, Jano Tomko wrote:
On 6/17/21 9:03 AM, Peter Krempa wrote:
On Wed, Jun 16, 2021 at 18:30:08 -0300, Daniel Henrique Barboza wrote:
Commit 56dcdec1ac81 ("conf: reject duplicate virtiofs tags") added
validation code to reject duplicated virtiofs tags. But the <target>
element is not always present, meaning that fs->dst can be NULL at that
point.
The tag should be mandatory, the test case in question was broken.
I see. That was my first guess, but then by reading the docs I got the
impression that the 'target' tag was optional for filesystem=mount.
In fact, by reading it again now, I see that other optional attributes
(e.g. binary) are marked as optional right on the start. Not sure why
I thought 'target' was optional first time I read this ...
It should be fixed now.
It is. Thanks!
Jano
If there is no "<target>" tag then the validation will fail in
virHashAddEntry() because fs->dst will be NULL. This is tested in
qemuxml2xml vhost-user-fs-sock, which is since then not passing:
1020) QEMU XML-2-XML-inactive vhost-user-fs-sock ... Expected result code=0 but received code=1
FAILED
1021) QEMU XML-2-XML-active vhost-user-fs-sock ... Expected result code=0 but received code=1
FAILED
The '<target>' tag is not mandatory, so let's consider that fs->dst
being NULL is a feasible scenario an adapt virDomainDefFSValidate()
accordingly.
Signed-off-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>
---
src/conf/domain_validate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 9422b00964..cf8b43b845 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -1504,7 +1504,7 @@ virDomainDefFSValidate(const virDomainDef *def)
for (i = 0; i < def->nfss; i++) {
const virDomainFSDef *fs = def->fss[i];
- if (fs->fsdriver != VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS)
+ if (fs->fsdriver != VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS || !fs->dst)
continue;
if (virHashHasEntry(dsts, fs->dst)) {
--
This should not be necessary once Jano pushes the patch moving the
validation of the presence of the virtiofs target, which was originally
before the patch that introduced the breakage, but I've requested
changes to it.
But lets see what Jano thinks about this.
