[libvirt] [PATCH 2/2] Do not generate security_model when fs driver is anything but 'path'

[Deepak C Shetty]

On 12/21/2011 08:11 PM, Daniel P. Berrange wrote:
> On Wed, Dec 21, 2011 at 11:17:17AM +0530, Deepak C Shetty wrote:
>> QEMU does not support security_model for anything but 'path' fs driver type.
>> Currently in libvirt, when security_model ( accessmode attribute) is not
>> specified it auto-generates it irrespective of the fs driver type. Also
>> when virt-manager (vmm) adds a new fs device with default security_model
>> the input xml passed to libvirt does not contain accessmode attribute, but
>> libvirt generates it as part of the virDomainDefine flow, which should
>> only be done if fs driver is of type 'path', else not. This patch fixes
>> these issues.
>>
>> Signed-off-by: Deepak C Shetty<deepakcs at linux.vnet.ibm.com>
>> ---
>>
>>   src/conf/domain_conf.c  |   13 +++++++++----
>>   src/qemu/qemu_command.c |   15 +++++++++------
>>   2 files changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 8b89a0b..2c91f82 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -10019,10 +10019,15 @@ virDomainFSDefFormat(virBufferPtr buf,
>>           return -1;
>>       }
>>
>> -
>> -    virBufferAsprintf(buf,
>> -                      "<filesystem type='%s' accessmode='%s'>\n",
>> -                      type, accessmode);
>> +    if (def->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH ||
>> +        def->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) {
>> +        virBufferAsprintf(buf,
>> +                          "<filesystem type='%s' accessmode='%s'>\n",
>> +                          type, accessmode);
>> +    } else {
>> +        virBufferAsprintf(buf,
>> +                          "<filesystem type='%s'>\n", type);
>> +    }
>
> No, this isn't right. We should *always* include the accessmode
> in the XML. Only at time of use should we decide whether the
> requested accessmode can be supported or not.
fsdriver type 'handle' does not support 'accessmode', if we include it
in the xml, wouldn't it be misleading at the xml level ?. Also when 
viewed from virsh/VMM, we do
not want to show accessmode attribute, if user selected fs driver type
as 'handle'. If we end up including accessmode in the xml always, it would
be misleading there too, correct ?
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index d33d7c8..1f70eb1 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -2004,12 +2004,15 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs,
>>       }
>>       virBufferAdd(&opt, driver, -1);
>>
>> -    if (fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_MAPPED) {
>> -        virBufferAddLit(&opt, ",security_model=mapped");
>> -    } else if(fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH) {
>> -        virBufferAddLit(&opt, ",security_model=passthrough");
>> -    } else if(fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_SQUASH) {
>> -        virBufferAddLit(&opt, ",security_model=none");
>> +    if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH ||
>> +        fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) {
>> +        if (fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_MAPPED) {
>> +            virBufferAddLit(&opt, ",security_model=mapped");
>> +        } else if(fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH) {
>> +            virBufferAddLit(&opt, ",security_model=passthrough");
>> +        } else if(fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_SQUASH) {
>> +            virBufferAddLit(&opt, ",security_model=none");
>> +        }
>>       }
> This is wrong too, because it is silently ignoring the accessmode
> that user requested in some cases. If a particular fsdriver does
> not support the requested accesmode, it should be raising a
> VIR_ERR_CONFIG_UNSUPPORTED error.
This typically won't happen when user is using VMM, since I took care
not to disable accessmode, when user selects fsdriver 'handle'. But its
possible using virsh, if someone edited the domain xml and added
fsdriver 'handle' with accessmode, in which case I agree, there should be
an error reported. Will send a v2 of this patch.
If we show unsupported error in this case, do we still include accessmode
in the xml ?, referring to your comment above, if we always include it
it will be misleading here, since virsh would show unsupported error
but xml would still include accessmode, is my understanding correct ?
> Daniel