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(a)gmail.com>
>> ---
>> 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.
>