4:37 a.m.
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
4:37 a.m.
New subject: [libvirt] [PATCH 01/11] qemuDomainAttachSCSIVHostDevice: Prefer qemuSecurity wrappers
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(a)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
6:58 a.m.
New subject: [libvirt] [PATCH 01/11] qemuDomainAttachSCSIVHostDevice: Prefer qemuSecurity wrappers
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(a)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.
4:37 a.m.
New subject: [libvirt] [PATCH 02/11] syntax-check: Enforce qemuSecurity
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(a)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
7:32 a.m.
New subject: [libvirt] [PATCH 02/11] syntax-check: Enforce qemuSecurity
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(a)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.
8:40 a.m.
New subject: [libvirt] [PATCH 02/11] syntax-check: Enforce qemuSecurity
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(a)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
4:37 a.m.
New subject: [libvirt] [PATCH 03/11] qemuDomainAttachSCSIVHostDevice: manage /dev entry
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(a)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
7:54 a.m.
New subject: [libvirt] [PATCH 03/11] qemuDomainAttachSCSIVHostDevice: manage /dev entry
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(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 8 ++++++++
1 file changed, 8 insertions(+)
ACK
4:37 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
Nearly all of these functions look the same. Except for a
different virSecurityManager API call. There is no need to copy
paste the code when we can use macros to generate it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
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
6:23 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
Nearly all of these functions look the same. Except for a
different virSecurityManager API call. There is no need to copy
paste the code when we can use macros to generate it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_security.c | 179 ++++++++++++-----------------------------------
1 file changed, 44 insertions(+), 135 deletions(-)
NACK, please don't partialy define function with macros.
6:37 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
On 02/08/2017 01:23 PM, Peter Krempa wrote:
On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
> Nearly all of these functions look the same. Except for a
> different virSecurityManager API call. There is no need to copy
> paste the code when we can use macros to generate it.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> 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
6:43 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
On 02/08/2017 01:23 PM, Peter Krempa wrote:
> On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
>> Nearly all of these functions look the same. Except for a
>> different virSecurityManager API call. There is no need to copy
>> paste the code when we can use macros to generate it.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> 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
7:37 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
On 02/08/2017 01:43 PM, Peter Krempa wrote:
On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
> On 02/08/2017 01:23 PM, Peter Krempa wrote:
>> On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
>>> Nearly all of these functions look the same. Except for a
>>> different virSecurityManager API call. There is no need to copy
>>> paste the code when we can use macros to generate it.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>> ---
>>> 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
7:52 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
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(a)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.
7:59 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
On Wed, Feb 08, 2017 at 02:37:48PM +0100, Michal Privoznik wrote:
On 02/08/2017 01:43 PM, Peter Krempa wrote:
> On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
>> On 02/08/2017 01:23 PM, Peter Krempa wrote:
>>> On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
>>>> Nearly all of these functions look the same. Except for a
>>>> different virSecurityManager API call. There is no need to copy
>>>> paste the code when we can use macros to generate it.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>>> ---
>>>> 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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:09 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
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 :)
8:33 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
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(a)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
8:49 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
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
8:55 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
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
9 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
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
2:52 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
On 02/08/2017 01:43 PM, Peter Krempa wrote:
On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
> On 02/08/2017 01:23 PM, Peter Krempa wrote:
>> On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
>>> Nearly all of these functions look the same. Except for a
>>> different virSecurityManager API call. There is no need to copy
>>> paste the code when we can use macros to generate it.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>> ---
>>> 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
3:52 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
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(a)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.
4:56 a.m.
New subject: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication
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(a)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
4:37 a.m.
New subject: [libvirt] [PATCH 05/11] qemu_security: Drop qemuSecuritySetRestoreAllLabelData struct
This struct is unused after 095f042ed68b01.
Signed-off-by: Michal Privoznik <mprivozn(a)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
7:21 a.m.
New subject: [libvirt] [PATCH 05/11] qemu_security: Drop qemuSecuritySetRestoreAllLabelData struct
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(a)redhat.com>
---
src/qemu/qemu_security.c | 9 ---------
1 file changed, 9 deletions(-)
ACK
4:37 a.m.
New subject: [libvirt] [PATCH 06/11] qemu_domain: Don't pass virDomainDeviceDefPtr to ns helpers
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(a)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
8:51 a.m.
New subject: [libvirt] [PATCH 06/11] qemu_domain: Don't pass virDomainDeviceDefPtr to ns helpers
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(a)redhat.com>
---
src/qemu/qemu_domain.c | 28 +++++-----------------------
1 file changed, 5 insertions(+), 23 deletions(-)
ACK
4:37 a.m.
New subject: [libvirt] [PATCH 07/11] qemuDomainNamespaceSetupDisk: Drop useless @src variable
Since its introduction in 81df21507bef9 this variable was never
used.
Signed-off-by: Michal Privoznik <mprivozn(a)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
7:26 a.m.
New subject: [libvirt] [PATCH 07/11] qemuDomainNamespaceSetupDisk: Drop useless @src variable
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(a)redhat.com>
---
src/qemu/qemu_domain.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
ACK
4:37 a.m.
New subject: [libvirt] [PATCH 08/11] qemuDomainNamespace{Setup, Teardown}Disk: Don't pass pointer to full disk
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(a)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
7:27 a.m.
New subject: [libvirt] [PATCH 08/11] qemuDomainNamespace{Setup, Teardown}Disk: Don't pass pointer to full disk
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(a)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
4:37 a.m.
New subject: [libvirt] [PATCH 09/11] qemuDomainDiskChainElement{Prepare, Revoke}: manage /dev entry
Again, one missed bit. This time without this commit there is no
/dev entry when doing disk snapshots.
Signed-off-by: Michal Privoznik <mprivozn(a)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
7:28 a.m.
New subject: [libvirt] [PATCH 09/11] qemuDomainDiskChainElement{Prepare, Revoke}: manage /dev entry
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(a)redhat.com>
---
src/qemu/qemu_domain.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
ACK
4:37 a.m.
New subject: [libvirt] [PATCH 10/11] qemu_security: Introduce ImageLabel APIs
Just like we need wrappers over other virSecurityManager APIs, we
need one for virSecurityManagerSetImageLabel and
virSecurityManagerRestoreImageLabel.
Signed-off-by: Michal Privoznik <mprivozn(a)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
7:29 a.m.
New subject: [libvirt] [PATCH 10/11] qemu_security: Introduce ImageLabel APIs
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(a)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.
4:37 a.m.
New subject: [libvirt] [PATCH 11/11] qemuDomainNamespaceSetupDisk: Simplify disk check
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(a)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
7:30 a.m.
New subject: [libvirt] [PATCH 11/11] qemuDomainNamespaceSetupDisk: Simplify disk check
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(a)redhat.com>
---
src/qemu/qemu_domain.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
ACK
9:04 a.m.
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
2879
days inactive
2880
days old
37 comments
3 participants
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Peter Krempa