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

[Martin Kletzander]

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);
}

or anything similar with the naming being done by someone else than me.

You can also extract the securityManager out of the vm pointer if you
want to make it even shorter.

>Michal
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170208/cf27ddbc/attachment-0001.sig>