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&gt; >>>>> --- >>>>> 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); }