[PATCH 1/1] domain_validate.c: fix virDomainDefFSValidate() when fs->dst is NULL

[Daniel Henrique Barboza]

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 at 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.
>>
>