Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Attachments:
- signature.asc (application/pgp-signature — 801 bytes)