[libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Michal Privoznik]

On 02/08/2017 02:59 PM, Martin Kletzander wrote:
> On Wed, Feb 08, 2017 at 02:37:48PM +0100, Michal Privoznik wrote:
>> On 02/08/2017 01:43 PM, Peter Krempa wrote:
>>> On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
>>>> On 02/08/2017 01:23 PM, Peter Krempa wrote:
>>>>> On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
>>>>>> Nearly all of these functions look the same. Except for a
>>>>>> different virSecurityManager API call. There is no need to copy
>>>>>> paste the code when we can use macros to generate it.
>>>>>>
>>>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>>>> ---
>>>>>>  src/qemu/qemu_security.c | 179
>>>>>> ++++++++++++-----------------------------------
>>>>>>  1 file changed, 44 insertions(+), 135 deletions(-)
>>>>>
>>>>> NACK, please don't partialy define function with macros.
>>>>>
>>>>
>>>> Why not? What is the downside?
>>>
> 
> I agree that macros that do a partial construct (start or end of a
> function) are really not readable.  Not talking about the navigation.
> 
>>> You'll never be able to navigate to the body of the function or ever
>>> find it try 'vim -t qemuSecurityRestoreHostdevLabel' or navigate to
>>> that after that patch.
>>
>> I don't think this is ultimate goal. A lot of our code is written in a
>> callback style: var->cb(blaah). You cannot jump to the actual function
>> either. If doing 'vim -t' is the ultimate goal then we should ban
>> callbacks too.
>>
>>>
>>> The downside of the code being totally unreadable is way worse than a
>>> few copied lines.
>>
>> Well, I was asked in other review to not copy the lines.
>> Also, the upside is that we can have a syntax-check rule that checks if
>> qemuSecurity wrapper is used instead of calling bare virSecurity...
>>
>> So what about:
>>
>> int
>> qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, ...)
>> {
>>   WRAP(RestoreHostdevLabel); /* macro that wraps
>> virSecurityManagerRestoreHostdevLabel */
>> }
>>
>> This way you can 'vim -t' into it. Although, the syntax-check rule is
>> going to be much more complex.
>>
> 
> How about simply:
> 
> int
> qemuNamespaceMountTransactionStart(virDomainObjPtr vm,
>                                   virSecurityManagerPtr secMgr)
> {
>    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>        virSecurityManagerTransactionStart(secMgr) < 0)
>        return -1;
> 
>    return 0;
> }
> 
> int
> qemuNamespaceMountTransactionCommit(virDomainObjPtr vm,
>                                    virSecurityManagerPtr secMgr)
> {
>    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>        virSecurityManagerTransactionCommit(secMgr, vm->pid) < 0)
>        return -1;
> 
>    return 0;
> }
> 
> void
> qemuNamespaceMountTransactionAbort(virSecurityManagerPtr secMgr)
> {
>    virSecurityManagerTransactionAbort(secMgr);
> }

This doesn't solve the syntax-check problem. But whatever, I will go
with this and just drop the syntax-check patch. We have plenty of
arguments for using macros here but since some don't like it I'm not
going to push it. Lets just hope that we will take care to use
qemuSecurity* wrappers instead of calling virSecurity APIs directly.
Honestly, I don't care that much.

Michal