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(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 35cdf50b0..b2155afcf 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -40,33 +40,49 @@ struct qemuSecuritySetRestoreAllLabelData {
};
-int
-qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- const char *stdin_path)
-{
- int ret = -1;
+#define PROLOGUE(F, type) \
+int \
+qemuSecurity##F(virQEMUDriverPtr driver, \
+ virDomainObjPtr vm, \
+ type var) \
+{ \
+ int ret = -1; \
+ \
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && \
+ virSecurityManagerTransactionStart(driver->securityManager) < 0) \
+ goto cleanup; \
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionStart(driver->securityManager) < 0)
- goto cleanup;
-
- if (virSecurityManagerSetAllLabel(driver->securityManager,
- vm->def,
- stdin_path) < 0)
- goto cleanup;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionCommit(driver->securityManager,
- vm->pid) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- virSecurityManagerTransactionAbort(driver->securityManager);
- return ret;
+#define EPILOGUE \
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && \
+ virSecurityManagerTransactionCommit(driver->securityManager, \
+ vm->pid) < 0) \
+ goto cleanup; \
+ \
+ ret = 0; \
+ cleanup: \
+ virSecurityManagerTransactionAbort(driver->securityManager); \
+ return ret; \
}
+#define WRAP1(F, type) \
+ PROLOGUE(F, type) \
+ if (virSecurityManager##F(driver->securityManager, \
+ vm->def, \
+ var) < 0) \
+ goto cleanup; \
+ \
+ EPILOGUE
+
+#define WRAP2(F, type) \
+ PROLOGUE(F, type) \
+ if (virSecurityManager##F(driver->securityManager, \
+ vm->def, \
+ var, NULL) < 0) \
+ goto cleanup; \
+ \
+ EPILOGUE
+
+WRAP1(SetAllLabel, const char *)
void
qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
@@ -85,115 +101,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
}
-int
-qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDiskDefPtr disk)
-{
- int ret = -1;
+WRAP1(SetDiskLabel, virDomainDiskDefPtr)
+WRAP1(RestoreDiskLabel, virDomainDiskDefPtr)
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionStart(driver->securityManager) < 0)
- goto cleanup;
-
- if (virSecurityManagerSetDiskLabel(driver->securityManager,
- vm->def,
- disk) < 0)
- goto cleanup;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionCommit(driver->securityManager,
- vm->pid) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- virSecurityManagerTransactionAbort(driver->securityManager);
- return ret;
-}
-
-
-int
-qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDiskDefPtr disk)
-{
- int ret = -1;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionStart(driver->securityManager) < 0)
- goto cleanup;
-
- if (virSecurityManagerRestoreDiskLabel(driver->securityManager,
- vm->def,
- disk) < 0)
- goto cleanup;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionCommit(driver->securityManager,
- vm->pid) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- virSecurityManagerTransactionAbort(driver->securityManager);
- return ret;
-}
-
-
-int
-qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainHostdevDefPtr hostdev)
-{
- int ret = -1;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionStart(driver->securityManager) < 0)
- goto cleanup;
-
- if (virSecurityManagerSetHostdevLabel(driver->securityManager,
- vm->def,
- hostdev,
- NULL) < 0)
- goto cleanup;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionCommit(driver->securityManager,
- vm->pid) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- virSecurityManagerTransactionAbort(driver->securityManager);
- return ret;
-}
-
-
-int
-qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainHostdevDefPtr hostdev)
-{
- int ret = -1;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionStart(driver->securityManager) < 0)
- goto cleanup;
-
- if (virSecurityManagerRestoreHostdevLabel(driver->securityManager,
- vm->def,
- hostdev,
- NULL) < 0)
- goto cleanup;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionCommit(driver->securityManager,
- vm->pid) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- virSecurityManagerTransactionAbort(driver->securityManager);
- return ret;
-}
+WRAP2(SetHostdevLabel, virDomainHostdevDefPtr)
+WRAP2(RestoreHostdevLabel, virDomainHostdevDefPtr)
--
2.11.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. The downside of the code being totally unreadable is way worse than a few copied lines. (YU|NA)CK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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.
>
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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?
>>
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);
}
or anything similar with the naming being done by someone else than me.
You can also extract the securityManager out of the vm pointer if you
want to make it even shorter.
>Michal
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Feb 08, 2017 at 14:59:13 +0100, Martin Kletzander wrote:
> On Wed, Feb 08, 2017 at 02:37:48PM +0100, Michal Privoznik wrote:
> > On 02/08/2017 01:43 PM, Peter Krempa wrote:
[...]
>
> 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);
> }
>
> or anything similar with the naming being done by someone else than me.
>
> You can also extract the securityManager out of the vm pointer if you
> want to make it even shorter.
Yes, this is exactly what I wanted to say in the parallel reply :)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 02/08/2017 02:59 PM, Martin Kletzander wrote:
> 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@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?
>>>
>
> 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);
> }
This doesn't solve the syntax-check problem. But whatever, I will go
with this and just drop the syntax-check patch. We have plenty of
arguments for using macros here but since some don't like it I'm not
going to push it. Lets just hope that we will take care to use
qemuSecurity* wrappers instead of calling virSecurity APIs directly.
Honestly, I don't care that much.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Feb 08, 2017 at 15:33:48 +0100, Michal Privoznik wrote:
> On 02/08/2017 02:59 PM, Martin Kletzander wrote:
> > 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:
[...]
>
> This doesn't solve the syntax-check problem. But whatever, I will go
> with this and just drop the syntax-check patch. We have plenty of
> arguments for using macros here but since some don't like it I'm not
> going to push it. Lets just hope that we will take care to use
> qemuSecurity* wrappers instead of calling virSecurity APIs directly.
> Honestly, I don't care that much.
You can do a macro:
#define QEMU_SECURITY_WRAPPED(func) func
And then use it in the functions that wrap the function like:
int
qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
int ret = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
virSecurityManagerTransactionStart(driver->securityManager) < 0)
goto cleanup;
if (QEMU_SECURITY_WRAPPED(virSecurityManagerRestoreDiskLabel)(driver->securityManager,
vm->def,
disk) < 0)
goto cleanup;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
virSecurityManagerTransactionCommit(driver->securityManager,
vm->pid) < 0)
goto cleanup;
ret = 0;
cleanup:
virSecurityManagerTransactionAbort(driver->securityManager);
return ret;
}
But the conditions are to use a very distinctive name of the macro and
the macro actually not doing much so that the readability of the code is
preserved, while having an anchor point for automatically looking up the
function names that were wrapped.
Yet another option would be to do:
#define QEMU_SECURITY_WRAPPED
void QEMU_SECURITY_WRAPPED qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
bool migrated);
In that case you need some 'sed' magic to figure out the function name
though.
Peter
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Feb 08, 2017 at 03:49:47PM +0100, Peter Krempa wrote:
>On Wed, Feb 08, 2017 at 15:33:48 +0100, Michal Privoznik wrote:
>> On 02/08/2017 02:59 PM, Martin Kletzander wrote:
>> > 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:
>
>[...]
>
>>
>> This doesn't solve the syntax-check problem. But whatever, I will go
>> with this and just drop the syntax-check patch. We have plenty of
>> arguments for using macros here but since some don't like it I'm not
>> going to push it. Lets just hope that we will take care to use
>> qemuSecurity* wrappers instead of calling virSecurity APIs directly.
>> Honestly, I don't care that much.
>
>You can do a macro:
>
>#define QEMU_SECURITY_WRAPPED(func) func
>
>And then use it in the functions that wrap the function like:
>
>int
>qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainDiskDefPtr disk)
>{
> int ret = -1;
>
> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> virSecurityManagerTransactionStart(driver->securityManager) < 0)
> goto cleanup;
>
> if (QEMU_SECURITY_WRAPPED(virSecurityManagerRestoreDiskLabel)(driver->securityManager,
> vm->def,
> disk) < 0)
> goto cleanup;
>
> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> virSecurityManagerTransactionCommit(driver->securityManager,
> vm->pid) < 0)
> goto cleanup;
>
> ret = 0;
> cleanup:
> virSecurityManagerTransactionAbort(driver->securityManager);
> return ret;
>}
>
>But the conditions are to use a very distinctive name of the macro and
>the macro actually not doing much so that the readability of the code is
>preserved, while having an anchor point for automatically looking up the
>function names that were wrapped.
>
>Yet another option would be to do:
>
>#define QEMU_SECURITY_WRAPPED
>
>void QEMU_SECURITY_WRAPPED qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> bool migrated);
>
>In that case you need some 'sed' magic to figure out the function name
>though.
>
Why not just take the wrappers around virSecurity code, put them into
qemu_security or similar and allow virSecurity* functions only in that
file? Clearly that file will only be wrappers, so it should help a lot.
>Peter
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 02/08/2017 03:55 PM, Martin Kletzander wrote:
> On Wed, Feb 08, 2017 at 03:49:47PM +0100, Peter Krempa wrote:
>> On Wed, Feb 08, 2017 at 15:33:48 +0100, Michal Privoznik wrote:
>>> On 02/08/2017 02:59 PM, Martin Kletzander wrote:
>>> > 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:
>>
>> [...]
>>
>>>
>>> This doesn't solve the syntax-check problem. But whatever, I will go
>>> with this and just drop the syntax-check patch. We have plenty of
>>> arguments for using macros here but since some don't like it I'm not
>>> going to push it. Lets just hope that we will take care to use
>>> qemuSecurity* wrappers instead of calling virSecurity APIs directly.
>>> Honestly, I don't care that much.
>>
>> You can do a macro:
>>
>> #define QEMU_SECURITY_WRAPPED(func) func
>>
>> And then use it in the functions that wrap the function like:
>>
>> int
>> qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> virDomainDiskDefPtr disk)
>> {
>> int ret = -1;
>>
>> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> virSecurityManagerTransactionStart(driver->securityManager) < 0)
>> goto cleanup;
>>
>> if
>> (QEMU_SECURITY_WRAPPED(virSecurityManagerRestoreDiskLabel)(driver->securityManager,
>>
>> vm->def,
>> disk)
>> < 0)
>> goto cleanup;
>>
>> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> virSecurityManagerTransactionCommit(driver->securityManager,
>> vm->pid) < 0)
>> goto cleanup;
>>
>> ret = 0;
>> cleanup:
>> virSecurityManagerTransactionAbort(driver->securityManager);
>> return ret;
>> }
>>
>> But the conditions are to use a very distinctive name of the macro and
>> the macro actually not doing much so that the readability of the code is
>> preserved, while having an anchor point for automatically looking up the
>> function names that were wrapped.
>>
>> Yet another option would be to do:
>>
>> #define QEMU_SECURITY_WRAPPED
>>
>> void QEMU_SECURITY_WRAPPED
>> qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> bool migrated);
>>
>> In that case you need some 'sed' magic to figure out the function name
>> though.
>>
>
> Why not just take the wrappers around virSecurity code, put them into
> qemu_security or similar and allow virSecurity* functions only in that
> file? Clearly that file will only be wrappers, so it should help a lot.
Because so far we don't have a qemuSecurity wrapper for every
virSecurity API. Not sure whether we will need a wrapper for all of them
(e.g. virSecurityManagerSetImageFDLabel)
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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. > > The downside of the code being totally unreadable is way worse than a > few copied lines. > > (YU|NA)CK > VIR_ONCE_GLOBAL_INIT KVM_FEATURE_DEF DECLARE_CLASS_COMMON src/esx/* tests/* XDR_PRIMITIVE_DISSECTOR VIR_ENUM_DECL VIR_ENUM_IMPL PROBE 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. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/09/2017 10:52 AM, Peter Krempa wrote: > 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@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. Doesn't matter. From 'vim -t' POV it's the same thing. Even from debugging POV it's the same thing (in gdb you'll see IR_CPU_x86_KVM_CLOCKSOURCE_cpuid array but you will not find it in sources). I don't see any difference, sorry. Since we want to have variable names generated by macros, and there is no difference to functions, I don't see the problem here. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.