[libvirt] [PATCH 00/11] qemu: Big namespace cleanup
Couple of these patches were requested in reviews to my previous namespace patches, couple of them are my own idea. At the same time, 3 bugs are fixed: 1) qemuSecurity wrappers are preferred over virSecurityManager 2) corresponding /dev entry is not created when attaching vhost scsi device 3) corresponding /dev entry is not created when doing blockjobs Michal Privoznik (11): qemuDomainAttachSCSIVHostDevice: Prefer qemuSecurity wrappers syntax-check: Enforce qemuSecurity qemuDomainAttachSCSIVHostDevice: manage /dev entry qemu_security: Kill code duplication qemu_security: Drop qemuSecuritySetRestoreAllLabelData struct qemu_domain: Don't pass virDomainDeviceDefPtr to ns helpers qemuDomainNamespaceSetupDisk: Drop useless @src variable qemuDomainNamespace{Setup,Teardown}Disk: Don't pass pointer to full disk qemuDomainDiskChainElement{Prepare,Revoke}: manage /dev entry qemu_security: Introduce ImageLabel APIs qemuDomainNamespaceSetupDisk: Simplify disk check cfg.mk | 8 ++ src/qemu/qemu_domain.c | 65 ++++++---------- src/qemu/qemu_domain.h | 4 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 20 +++-- src/qemu/qemu_security.c | 189 ++++++++++++----------------------------------- src/qemu/qemu_security.h | 8 ++ 7 files changed, 102 insertions(+), 194 deletions(-) -- 2.11.0
Since we have qemuSecurity wrappers over virSecurityManagerSetHostdevLabel and virSecurityManagerRestoreHostdevLabel we ought to use them instead of calling secdriver APIs directly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e272df356..dd6e31823 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2552,8 +2552,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, goto cleanup; teardowncgroup = true; - if (virSecurityManagerSetHostdevLabel(driver->securityManager, - vm->def, hostdev, NULL) < 0) + if (qemuSecuritySetHostdevLabel(driver, vm, hostdev) < 0) goto cleanup; teardownlabel = true; @@ -2612,8 +2611,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); if (teardownlabel && - virSecurityManagerRestoreHostdevLabel(driver->securityManager, - vm->def, hostdev, NULL) < 0) + qemuSecurityRestoreHostdevLabel(driver, vm, hostdev) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); if (releaseaddr) qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); -- 2.11.0
On Wed, Feb 08, 2017 at 11:37:04 +0100, Michal Privoznik wrote:
Since we have qemuSecurity wrappers over virSecurityManagerSetHostdevLabel and virSecurityManagerRestoreHostdevLabel we ought to use them instead of calling secdriver APIs directly.
Also it possibly would be worth mentioning that without those wrappers the labelling won't be done in the correct namespace and thus won't apply to the nodes seen by qemu itself. I presume that that bug actually motivated you do do so.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e272df356..dd6e31823 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2552,8 +2552,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, goto cleanup; teardowncgroup = true;
- if (virSecurityManagerSetHostdevLabel(driver->securityManager, - vm->def, hostdev, NULL) < 0) + if (qemuSecuritySetHostdevLabel(driver, vm, hostdev) < 0) goto cleanup; teardownlabel = true;
@@ -2612,8 +2611,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); if (teardownlabel && - virSecurityManagerRestoreHostdevLabel(driver->securityManager, - vm->def, hostdev, NULL) < 0) + qemuSecurityRestoreHostdevLabel(driver, vm, hostdev) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); if (releaseaddr) qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
ACK with commit message fixed.
Now that we have some qemuSecurity wrappers over virSecurityManager APIs, lets make sure everybody sticks with them. We have them for a reason and calling virSecurityManager API directly instead of wrapper may lead into accidentally labelling a file on the host instead of namespace. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cfg.mk b/cfg.mk index 69e3f3a1a..6fb2fc961 100644 --- a/cfg.mk +++ b/cfg.mk @@ -585,6 +585,14 @@ sc_prohibit_unsigned_pid: halt='use signed type for pid values' \ $(_sc_search_regexp) +sc_prohibit_direct_secdriver: + @for i in $$(grep -i ^WRAP.\( src/qemu/qemu_security.c | \ + awk 'BEGIN {FS = "[^[:alnum:]]"} {print "virSecurityManager" $$2 }'); do \ + grep -n $$i $$($(VC_LIST_EXCEPT) | grep -E '^src/qemu/') && \ + { echo "$(ME): prefer qemuSecurity$${i#virSecurityManager} over $$i" 1>&2; exit 1; } \ + done || : + + # Many of the function names below came from this filter: # git grep -B2 '\<_('|grep -E '\.c- *[[:alpha:]_][[:alnum:]_]* ?\(.*[,;]$' \ # |sed 's/.*\.c- *//'|perl -pe 's/ ?\(.*//'|sort -u \ -- 2.11.0
On Wed, Feb 08, 2017 at 11:37:05 +0100, Michal Privoznik wrote:
Now that we have some qemuSecurity wrappers over virSecurityManager APIs, lets make sure everybody sticks with them. We have them for a reason and calling virSecurityManager API directly instead of wrapper may lead into accidentally labelling a file on the host instead of namespace.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/cfg.mk b/cfg.mk index 69e3f3a1a..6fb2fc961 100644 --- a/cfg.mk +++ b/cfg.mk @@ -585,6 +585,14 @@ sc_prohibit_unsigned_pid: halt='use signed type for pid values' \ $(_sc_search_regexp)
+sc_prohibit_direct_secdriver: + @for i in $$(grep -i ^WRAP.\( src/qemu/qemu_security.c | \ + awk 'BEGIN {FS = "[^[:alnum:]]"} {print "virSecurityManager" $$2 }'); do \ + grep -n $$i $$($(VC_LIST_EXCEPT) | grep -E '^src/qemu/') && \ + { echo "$(ME): prefer qemuSecurity$${i#virSecurityManager} over $$i" 1>&2; exit 1; } \ + done || :
This won't work without the "WRAP" stuff so you'll need to come up with something else.
On 02/08/2017 02:32 PM, Peter Krempa wrote:
On Wed, Feb 08, 2017 at 11:37:05 +0100, Michal Privoznik wrote:
Now that we have some qemuSecurity wrappers over virSecurityManager APIs, lets make sure everybody sticks with them. We have them for a reason and calling virSecurityManager API directly instead of wrapper may lead into accidentally labelling a file on the host instead of namespace.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/cfg.mk b/cfg.mk index 69e3f3a1a..6fb2fc961 100644 --- a/cfg.mk +++ b/cfg.mk @@ -585,6 +585,14 @@ sc_prohibit_unsigned_pid: halt='use signed type for pid values' \ $(_sc_search_regexp)
+sc_prohibit_direct_secdriver: + @for i in $$(grep -i ^WRAP.\( src/qemu/qemu_security.c | \ + awk 'BEGIN {FS = "[^[:alnum:]]"} {print "virSecurityManager" $$2 }'); do \ + grep -n $$i $$($(VC_LIST_EXCEPT) | grep -E '^src/qemu/') && \ + { echo "$(ME): prefer qemuSecurity$${i#virSecurityManager} over $$i" 1>&2; exit 1; } \ + done || :
This won't work without the "WRAP" stuff so you'll need to come up with something else.
Without WRAP it's going to be super tricky as I'd have try to match functions from qemu_security.h with those from security_manager.h. If you have some bright idea, please do share it, because frankly I'm out of them. Michal
Again, one missed bit. This time without this commit there is no /dev entry when attaching vhost SCSI device. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index dd6e31823..778c8ef20 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2532,6 +2532,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, char *devstr = NULL; bool teardowncgroup = false; bool teardownlabel = false; + bool teardowndevice = false; bool releaseaddr = false; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { @@ -2548,6 +2549,10 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, return -1; } + if (qemuDomainNamespaceSetupHostdev(driver, vm, hostdev) < 0) + goto cleanup; + teardowndevice = true; + if (qemuSetupHostdevCgroup(vm, hostdev) < 0) goto cleanup; teardowncgroup = true; @@ -2613,6 +2618,9 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, if (teardownlabel && qemuSecurityRestoreHostdevLabel(driver, vm, hostdev) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); + if (teardowndevice && + qemuDomainNamespaceTeardownHostdev(driver, vm, hostdev) < 0) + VIR_WARN("Unable to remove host device from /dev"); if (releaseaddr) qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); } -- 2.11.0
On Wed, Feb 08, 2017 at 11:37:06 +0100, Michal Privoznik wrote:
Again, one missed bit. This time without this commit there is no /dev entry when attaching vhost SCSI device.
Commit message is a bit confusing. So the /dev/ entry in the namespace of the qemu process is not created when attaching scsi disks.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 8 ++++++++ 1 file changed, 8 insertions(+)
ACK
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
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.
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
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
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
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.
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
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 :)
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
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
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
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
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
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.
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
This struct is unused after 095f042ed68b01. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index b2155afcf..06bff2470 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -31,15 +31,6 @@ VIR_LOG_INIT("qemu.qemu_process"); -struct qemuSecuritySetRestoreAllLabelData { - bool set; - virQEMUDriverPtr driver; - virDomainObjPtr vm; - const char *stdin_path; - bool migrated; -}; - - #define PROLOGUE(F, type) \ int \ qemuSecurity##F(virQEMUDriverPtr driver, \ -- 2.11.0
On Wed, Feb 08, 2017 at 11:37:08 +0100, Michal Privoznik wrote:
This struct is unused after 095f042ed68b01.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 9 --------- 1 file changed, 9 deletions(-)
ACK
There is no need for this. None of the namespace helpers uses it. Historically it was used when calling secdriver APIs, but we don't to that anymore. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 28 +++++----------------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 54e63878f..067b7a42f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7643,7 +7643,6 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver, struct qemuDomainAttachDeviceMknodData { virQEMUDriverPtr driver; virDomainObjPtr vm; - virDomainDeviceDefPtr devDef; const char *file; const char *target; struct stat sb; @@ -7747,7 +7746,6 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, static int qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr devDef, const char *file, unsigned int ttl) { @@ -7767,7 +7765,6 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver, data.driver = driver; data.vm = vm; - data.devDef = devDef; data.file = file; if (lstat(file, &data.sb) < 0) { @@ -7840,8 +7837,7 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver, } if (isLink && - qemuDomainAttachDeviceMknodRecursive(driver, vm, devDef, - target, ttl -1) < 0) + qemuDomainAttachDeviceMknodRecursive(driver, vm, target, ttl -1) < 0) goto cleanup; ret = 0; @@ -7858,13 +7854,11 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver, static int qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr devDef, const char *file) { long symloop_max = sysconf(_SC_SYMLOOP_MAX); - return qemuDomainAttachDeviceMknodRecursive(driver, vm, devDef, - file, symloop_max); + return qemuDomainAttachDeviceMknodRecursive(driver, vm, file, symloop_max); } @@ -7888,7 +7882,6 @@ qemuDomainDetachDeviceUnlinkHelper(pid_t pid ATTRIBUTE_UNUSED, static int qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, virDomainObjPtr vm, - virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, const char *file) { if (virProcessRunInMountNamespace(vm->pid, @@ -7905,7 +7898,6 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { - virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_DISK, .data.disk = disk}; virStorageSourcePtr next; const char *src = NULL; struct stat sb; @@ -7935,7 +7927,6 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, if (qemuDomainAttachDeviceMknod(driver, vm, - &dev, next->path) < 0) goto cleanup; } @@ -7966,7 +7957,6 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_HOSTDEV, .data.hostdev = hostdev}; int ret = -1; char *path = NULL; @@ -7984,7 +7974,6 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver, if (qemuDomainAttachDeviceMknod(driver, vm, - &dev, path) < 0) goto cleanup; ret = 0; @@ -7999,7 +7988,6 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_HOSTDEV, .data.hostdev = hostdev}; int ret = -1; char *path = NULL; @@ -8015,7 +8003,7 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver, goto cleanup; } - if (qemuDomainDetachDeviceUnlink(driver, vm, &dev, path) < 0) + if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0) goto cleanup; ret = 0; @@ -8030,7 +8018,6 @@ qemuDomainNamespaceSetupChardev(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr) { - virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_CHR, .data.chr = chr}; const char *path; int ret = -1; @@ -8044,7 +8031,6 @@ qemuDomainNamespaceSetupChardev(virQEMUDriverPtr driver, if (qemuDomainAttachDeviceMknod(driver, vm, - &dev, path) < 0) goto cleanup; ret = 0; @@ -8058,7 +8044,6 @@ qemuDomainNamespaceTeardownChardev(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr) { - virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_CHR, .data.chr = chr}; int ret = -1; const char *path = NULL; @@ -8070,7 +8055,7 @@ qemuDomainNamespaceTeardownChardev(virQEMUDriverPtr driver, path = chr->source->data.file.path; - if (qemuDomainDetachDeviceUnlink(driver, vm, &dev, path) < 0) + if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0) goto cleanup; ret = 0; @@ -8084,7 +8069,6 @@ qemuDomainNamespaceSetupRNG(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng) { - virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_RNG, .data.rng = rng}; const char *path = NULL; int ret = -1; @@ -8104,7 +8088,6 @@ qemuDomainNamespaceSetupRNG(virQEMUDriverPtr driver, if (qemuDomainAttachDeviceMknod(driver, vm, - &dev, path) < 0) goto cleanup; ret = 0; @@ -8118,7 +8101,6 @@ qemuDomainNamespaceTeardownRNG(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng) { - virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_RNG, .data.rng = rng}; int ret = -1; const char *path = NULL; @@ -8136,7 +8118,7 @@ qemuDomainNamespaceTeardownRNG(virQEMUDriverPtr driver, goto cleanup; } - if (qemuDomainDetachDeviceUnlink(driver, vm, &dev, path) < 0) + if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0) goto cleanup; ret = 0; -- 2.11.0
On Wed, Feb 08, 2017 at 11:37:09 +0100, Michal Privoznik wrote:
There is no need for this. None of the namespace helpers uses it. Historically it was used when calling secdriver APIs, but we don't to that anymore.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 28 +++++----------------------- 1 file changed, 5 insertions(+), 23 deletions(-)
ACK
Since its introduction in 81df21507bef9 this variable was never used. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 067b7a42f..8ec9601d2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7899,7 +7899,6 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { virStorageSourcePtr next; - const char *src = NULL; struct stat sb; int ret = -1; @@ -7914,14 +7913,14 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, if (stat(next->path, &sb) < 0) { virReportSystemError(errno, - _("Unable to access %s"), src); + _("Unable to access %s"), next->path); goto cleanup; } if (!S_ISBLK(sb.st_mode)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Disk source %s must be a block device"), - src); + next->path); goto cleanup; } -- 2.11.0
On Wed, Feb 08, 2017 at 11:37:10 +0100, Michal Privoznik wrote:
Since its introduction in 81df21507bef9 this variable was never used.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
ACK
These functions do not need to see the whole virDomainDiskDef. Moreover, they are going to be called from places where we don't have access to the full disk definition. Sticking with virStorageSource is more than enough. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++---- src/qemu/qemu_domain.h | 4 ++-- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8ec9601d2..5db8b60c5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7896,7 +7896,7 @@ qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, int qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk) + virStorageSourcePtr src) { virStorageSourcePtr next; struct stat sb; @@ -7905,8 +7905,8 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; - for (next = disk->src; next; next = next->backingStore) { - if (!next->path || !virStorageSourceIsBlockLocal(disk->src)) { + for (next = src; next; next = next->backingStore) { + if (!next->path || !virStorageSourceIsBlockLocal(src)) { /* Not creating device. Just continue. */ continue; } @@ -7939,7 +7939,7 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, int qemuDomainNamespaceTeardownDisk(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, virDomainObjPtr vm ATTRIBUTE_UNUSED, - virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) + virStorageSourcePtr src ATTRIBUTE_UNUSED) { /* While in hotplug case we create the whole backing chain, * here we must limit ourselves. The disk we want to remove diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 39731826e..5cfa3e114 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -810,11 +810,11 @@ int qemuDomainCreateNamespace(virQEMUDriverPtr driver, int qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk); + virStorageSourcePtr src); int qemuDomainNamespaceTeardownDisk(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk); + virStorageSourcePtr src); int qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 37ccfdf6b..89bc833de 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15915,7 +15915,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, if (disk->mirror->format && disk->mirror->format != VIR_STORAGE_FILE_RAW && - (qemuDomainNamespaceSetupDisk(driver, vm, disk) < 0 || + (qemuDomainNamespaceSetupDisk(driver, vm, disk->src) < 0 || qemuSetupDiskCgroup(vm, disk) < 0 || qemuSecuritySetDiskLabel(driver, vm, disk) < 0)) goto cleanup; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 778c8ef20..2f209f12b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -110,7 +110,7 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver, vm, disk) < 0) goto cleanup; - if (qemuDomainNamespaceSetupDisk(driver, vm, disk) < 0) + if (qemuDomainNamespaceSetupDisk(driver, vm, disk->src) < 0) goto rollback_lock; if (qemuSecuritySetDiskLabel(driver, vm, disk) < 0) @@ -132,7 +132,7 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver, virDomainDiskGetSource(disk)); rollback_namespace: - if (qemuDomainNamespaceTeardownDisk(driver, vm, disk) < 0) + if (qemuDomainNamespaceTeardownDisk(driver, vm, disk->src) < 0) VIR_WARN("Unable to remove /dev entry for %s", virDomainDiskGetSource(disk)); @@ -3649,7 +3649,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", src); - if (qemuDomainNamespaceTeardownDisk(driver, vm, disk) < 0) + if (qemuDomainNamespaceTeardownDisk(driver, vm, disk->src) < 0) VIR_WARN("Unable to remove /dev entry for %s", src); dev.type = VIR_DOMAIN_DEVICE_DISK; -- 2.11.0
On Wed, Feb 08, 2017 at 11:37:11 +0100, Michal Privoznik wrote:
These functions do not need to see the whole virDomainDiskDef. Moreover, they are going to be called from places where we don't have access to the full disk definition. Sticking with virStorageSource is more than enough.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++---- src/qemu/qemu_domain.h | 4 ++-- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-)
ACK
Again, one missed bit. This time without this commit there is no /dev entry when doing disk snapshots. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5db8b60c5..9e34d73be 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5090,14 +5090,17 @@ qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr elem) { - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, elem) < 0) - VIR_WARN("Unable to restore security label on %s", NULLSTR(elem->path)); - if (qemuTeardownImageCgroup(vm, elem) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(elem->path)); + if (virSecurityManagerRestoreImageLabel(driver->securityManager, + vm->def, elem) < 0) + VIR_WARN("Unable to restore security label on %s", NULLSTR(elem->path)); + + if (qemuDomainNamespaceTeardownDisk(driver, vm, elem) < 0) + VIR_WARN("Unable to remove /dev entry for %s", NULLSTR(elem->path)); + if (virDomainLockImageDetach(driver->lockManager, vm, elem) < 0) VIR_WARN("Unable to release lock on %s", NULLSTR(elem->path)); } @@ -5126,6 +5129,9 @@ qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, elem) < 0) goto cleanup; + if (qemuDomainNamespaceSetupDisk(driver, vm, elem) < 0) + goto cleanup; + if (qemuSetupImageCgroup(vm, elem) < 0) goto cleanup; -- 2.11.0
On Wed, Feb 08, 2017 at 11:37:12 +0100, Michal Privoznik wrote:
Again, one missed bit. This time without this commit there is no /dev entry when doing disk snapshots.
Snapshots and block-copy. Both of them start using a new image file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
ACK
Just like we need wrappers over other virSecurityManager APIs, we need one for virSecurityManagerSetImageLabel and virSecurityManagerRestoreImageLabel. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 7 +++---- src/qemu/qemu_security.c | 3 +++ src/qemu/qemu_security.h | 8 ++++++++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9e34d73be..a4ee652db 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -31,6 +31,7 @@ #include "qemu_parse_command.h" #include "qemu_capabilities.h" #include "qemu_migration.h" +#include "qemu_security.h" #include "viralloc.h" #include "virlog.h" #include "virerror.h" @@ -5094,8 +5095,7 @@ qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(elem->path)); - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, elem) < 0) + if (qemuSecurityRestoreImageLabel(driver, vm, elem) < 0) VIR_WARN("Unable to restore security label on %s", NULLSTR(elem->path)); if (qemuDomainNamespaceTeardownDisk(driver, vm, elem) < 0) @@ -5135,8 +5135,7 @@ qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, if (qemuSetupImageCgroup(vm, elem) < 0) goto cleanup; - if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def, - elem) < 0) + if (qemuSecuritySetImageLabel(driver, vm, elem) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 06bff2470..131be6e4b 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -95,5 +95,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, WRAP1(SetDiskLabel, virDomainDiskDefPtr) WRAP1(RestoreDiskLabel, virDomainDiskDefPtr) +WRAP1(SetImageLabel, virStorageSourcePtr) +WRAP1(RestoreImageLabel, virStorageSourcePtr) + WRAP2(SetHostdevLabel, virDomainHostdevDefPtr) WRAP2(RestoreHostdevLabel, virDomainHostdevDefPtr) diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index cc373b3e1..54638908d 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -45,6 +45,14 @@ int qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk); +int qemuSecuritySetImageLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src); + +int qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src); + int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev); -- 2.11.0
On Wed, Feb 08, 2017 at 11:37:13 +0100, Michal Privoznik wrote:
Just like we need wrappers over other virSecurityManager APIs, we need one for virSecurityManagerSetImageLabel and virSecurityManagerRestoreImageLabel.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 7 +++---- src/qemu/qemu_security.c | 3 +++ src/qemu/qemu_security.h | 8 ++++++++ 3 files changed, 14 insertions(+), 4 deletions(-)
[...]
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 06bff2470..131be6e4b 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -95,5 +95,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, WRAP1(SetDiskLabel, virDomainDiskDefPtr) WRAP1(RestoreDiskLabel, virDomainDiskDefPtr)
+WRAP1(SetImageLabel, virStorageSourcePtr) +WRAP1(RestoreImageLabel, virStorageSourcePtr) + WRAP2(SetHostdevLabel, virDomainHostdevDefPtr) WRAP2(RestoreHostdevLabel, virDomainHostdevDefPtr)
Obviously NACK to this.
Firstly, instead of checking for next->path the virStorageSourceIsEmpty() function should be used which also takes disk type into account. Secondly, not every disk source passed has the correct type set (due to our laziness). Therefore, instead of checking for virStorageSourceIsBlockLocal() and also S_ISBLK() the former can be refined to just virStorageSourceIsLocalStorage(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a4ee652db..7c696963e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7911,7 +7911,8 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, return 0; for (next = src; next; next = next->backingStore) { - if (!next->path || !virStorageSourceIsBlockLocal(src)) { + if (virStorageSourceIsEmpty(next) || + !virStorageSourceIsLocalStorage(next)) { /* Not creating device. Just continue. */ continue; } @@ -7922,12 +7923,8 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, goto cleanup; } - if (!S_ISBLK(sb.st_mode)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Disk source %s must be a block device"), - next->path); - goto cleanup; - } + if (!S_ISBLK(sb.st_mode)) + continue; if (qemuDomainAttachDeviceMknod(driver, vm, -- 2.11.0
On Wed, Feb 08, 2017 at 11:37:14 +0100, Michal Privoznik wrote:
Firstly, instead of checking for next->path the virStorageSourceIsEmpty() function should be used which also takes disk type into account. Secondly, not every disk source passed has the correct type set (due to our laziness). Therefore, instead of checking for virStorageSourceIsBlockLocal() and also S_ISBLK() the former can be refined to just virStorageSourceIsLocalStorage().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
ACK
On 02/08/2017 11:37 AM, Michal Privoznik wrote:
Couple of these patches were requested in reviews to my previous namespace patches, couple of them are my own idea. At the same time, 3 bugs are fixed: 1) qemuSecurity wrappers are preferred over virSecurityManager 2) corresponding /dev entry is not created when attaching vhost scsi device 3) corresponding /dev entry is not created when doing blockjobs
Michal Privoznik (11): qemuDomainAttachSCSIVHostDevice: Prefer qemuSecurity wrappers syntax-check: Enforce qemuSecurity qemuDomainAttachSCSIVHostDevice: manage /dev entry qemu_security: Kill code duplication qemu_security: Drop qemuSecuritySetRestoreAllLabelData struct qemu_domain: Don't pass virDomainDeviceDefPtr to ns helpers qemuDomainNamespaceSetupDisk: Drop useless @src variable qemuDomainNamespace{Setup,Teardown}Disk: Don't pass pointer to full disk qemuDomainDiskChainElement{Prepare,Revoke}: manage /dev entry qemu_security: Introduce ImageLabel APIs qemuDomainNamespaceSetupDisk: Simplify disk check
cfg.mk | 8 ++ src/qemu/qemu_domain.c | 65 ++++++---------- src/qemu/qemu_domain.h | 4 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 20 +++-- src/qemu/qemu_security.c | 189 ++++++++++++----------------------------------- src/qemu/qemu_security.h | 8 ++ 7 files changed, 102 insertions(+), 194 deletions(-)
FYI, I've pushed the ACKed patches. For a subset of not ACKed patches I will post v2 in a minute. Michal
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Peter Krempa