[PATCH] Don't require secdrivers to implement .domainMoveImageMetadata

[Michal Privoznik]

On 5/18/20 8:16 AM, Erik Skultety wrote:
> On Fri, May 15, 2020 at 05:44:38PM +0200, Michal Privoznik wrote:
>> The AppArmor secdriver does not use labels to grant access to
>> resources. Therefore, it doesn't use XATTRs and hence it lacks
>> implementation of .domainMoveImageMetadata callback. This leads
>> to a harmless but needless error message appearing in the logs:
>>
>>    virSecurityManagerMoveImageMetadata:476 : this function is not
>>    supported by the connection driver: virSecurityManagerMoveImageMetadata
>>
>> Closes: https://gitlab.com/libvirt/libvirt/-/issues/25
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/security/security_manager.c |  3 +--
>>   src/security/security_nop.c     | 10 ----------
>>   2 files changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
>> index 2dea294784..b1237d63b6 100644
>> --- a/src/security/security_manager.c
>> +++ b/src/security/security_manager.c
>> @@ -473,8 +473,7 @@ virSecurityManagerMoveImageMetadata(virSecurityManagerPtr mgr,
>>           return ret;
>>       }
>>   
>> -    virReportUnsupportedError();
>> -    return -1;
>> +    return 0;
>>   }
> 
> To ^this hunk:
> Reviewed-by: Erik Skultety <eskultet at redhat.com>
> 
>>   
>>   
>> diff --git a/src/security/security_nop.c b/src/security/security_nop.c
>> index c1856eb421..d5f715b916 100644
>> --- a/src/security/security_nop.c
>> +++ b/src/security/security_nop.c
>> @@ -225,15 +225,6 @@ virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
>>       return 0;
>>   }
>>   
>> -static int
>> -virSecurityDomainMoveImageMetadataNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
>> -                                      pid_t pid G_GNUC_UNUSED,
>> -                                      virStorageSourcePtr src G_GNUC_UNUSED,
>> -                                      virStorageSourcePtr dst G_GNUC_UNUSED)
>> -{
>> -    return 0;
>> -}
>> -
> 
> ^This is an unrelated change and I also think that the Nop driver should
> implement (ideally) all the functions, so I don't see a reason in removing
> this one, otherwise we should remove more than just this very function.
> 

It's not unrelated. The NOP driver is always present (see 
security_drivers[] in src/security/security_driver.c). Therefore, this 
dummy implementation was needed because if no other driver was loaded 
then NOP implementation saved us from reporting unsupported error.

But you are right that the important bit is the first hunk. So whatever 
you prefer.

Michal