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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
>>>> ---
>>>>  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?
> 
> 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.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux