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

[Peter Krempa]

On Wed, Feb 08, 2017 at 14:37:48 +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?
> > 
> > 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.

Callbacks are way different from this case. Even if you have a callback
then a debugger prints the function name. The same applies for logs.

With a macro that defines a function you get a function name that is not
present in the source without pre-processing it. With the callback you
still have the symbol, while the call path may be opaque.

> 
> > 
> > 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...

I don't think that the macros are a requirement to have a syntax check.

> 
> So what about:
> 
> int
> qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, ...)
> {
>    WRAP(RestoreHostdevLabel); /* macro that wraps
> virSecurityManagerRestoreHostdevLabel */

I'd extract the "PROLOGUE" and "EPILOGUE" parts as a function and then
just call the wrapped function directly. I don't see a point to have a
macro here.

> }
> 
> This way you can 'vim -t' into it. Although, the syntax-check rule is
> going to be much more complex.

Just wrap everything and outlaw it outside of this code then.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170208/0445adad/attachment-0001.sig>