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

[Peter Krempa]

On Thu, Feb 09, 2017 at 09:52:03 +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.
> > 
> > The downside of the code being totally unreadable is way worse than a
> > few copied lines.
> > 
> > (YU|NA)CK
> > 
> 
> VIR_ONCE_GLOBAL_INIT

I've hit these a few times. In this case it irritates me that I can't
see the ...Initialize function.

> KVM_FEATURE_DEF

This does not create functions.

> DECLARE_CLASS_COMMON

This does not create functions. The variables accessed are global and
declared without any macro All usage is within one screen. Only opaque
function call is to the ...Dispose function. All functions can be viewed
with vim -t. The macros are undefined within the single case where it's
used.

This does not help your example.

> src/esx/*

Don't care, never used ESX.

> tests/*

Granted, in tests there are quite a few questionable usages of macros.
One of the worst examples is TEST_CHAIN. Tests are usually "fun" to
debug.

> XDR_PRIMITIVE_DISSECTOR

Don't care, never used the dissector really.

> VIR_ENUM_DECL
> VIR_ENUM_IMPL

I'm hitting these regularly, but they are so common that I got used to
it and also it's pretty easy to figure out the type name you want to
access. Seeing the code in this case would not help that much though.
It's far more useful to see the enum or the strings belonging to it.

The only option I see here is if we'd generate them into a separate file
rather than relying on macros. But as I've said that would not help
much since you want to see the strings.

> PROBE

This macro uses __LINE__ and __FILE__ stuff so it can't be avoided. Also
if you don't enable DTRACE the macro does not expand to anything, which
is the default.

> just to name a few places where we already do what you NACKed. If using
> macros to construct functions is that bad, I expect you to write a patch
> to get rid of those.

No I won't fix those just because I rejected your code.

Reasoning that something should be allowed because we have prior art is
weak. We don't have to make the code uglier than it is.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170209/9bce7557/attachment-0001.sig>