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