On 11.06.2012 18:54, Eric Blake wrote:
> On 06/11/2012 08:29 AM, Michal Privoznik wrote:
>> Currently, if qemuProcessStart fail at some point, e.g. because
>> domain being started wants a PCI/USB device already assigned to
>> a different domain, we jump to cleanup label where qemuProcessStop
>> is performed. This unconditionally calls virSecurityManagerRestoreAllLabel
>> which is wrong because the other domain is still using those devices.
>>
>> However, once we successfully label all devices/paths in
>> qemuProcessStart() from that point on, we have to perform a rollback
>> on failure - that is - we have to virSecurityManagerRestoreAllLabel.
>> ---
>> src/qemu/qemu_process.c | 12 ++++++++----
>> src/qemu/qemu_process.h | 3 ++-
>> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> Double-negative logic. But I guess we're stuck with it, as the default
> of 'flags==0' must imply the relabel.
>
>> @@ -3984,9 +3987,10 @@ void qemuProcessStop(struct qemud_driver *driver,
>> }
>>
>> /* Reset Security Labels */
>> - virSecurityManagerRestoreAllLabel(driver->securityManager,
>> - vm->def,
>> - flags & VIR_QEMU_PROCESS_STOP_MIGRATED);
>> + if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL))
>
> Took me a couple reads to convince myself that I couldn't come up with
> any nicer wording of this condition without breaking defaults.
>
> ACK.
>
Yeah, it is quite confusing, therefore I am squashing in some comments
before pushing:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 0309bfb..5f3aa99 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3281,7 +3281,7 @@ int qemuProcessStart(virConnectPtr conn,
int i;
char *nodeset = NULL;
char *nodemask = NULL;
- unsigned int stop_flags = VIR_QEMU_PROCESS_STOP_NO_RELABEL;
+ unsigned int stop_flags;
/* Okay, these are just internal flags,
* but doesn't hurt to check */
@@ -3289,6 +3289,12 @@ int qemuProcessStart(virConnectPtr conn,
VIR_QEMU_PROCESS_START_PAUSED |
VIR_QEMU_PROCESS_START_AUTODESROY, -1);
+ /* From now on until domain security labeling is done:
+ * if any operation fails and we goto cleanup, we must not
+ * restore any security label as we would overwrite labels
+ * we did not set. */
+ stop_flags = VIR_QEMU_PROCESS_STOP_NO_RELABEL;
+
hookData.conn = conn;
hookData.vm = vm;
hookData.driver = driver;
@@ -3632,6 +3638,10 @@ int qemuProcessStart(virConnectPtr conn,
vm->def, stdin_path) < 0)
goto cleanup;
+ /* Security manager labeled all devices, therefore
+ * if any operation from now on fails and we goto cleanup,
+ * where virSecurityManagerRestoreAllLabel() is called
+ * (hidden under qemuProcessStop) we need to restore labels. */
stop_flags &= ~VIR_QEMU_PROCESS_STOP_NO_RELABEL;
if (stdin_fd != -1) {
@@ -3986,7 +3996,7 @@ void qemuProcessStop(struct qemud_driver *driver,
VIR_FREE(xml);
}
- /* Reset Security Labels */
+ /* Reset Security Labels unless caller don't want us to */
if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL))
virSecurityManagerRestoreAllLabel(driver->securityManager,
vm->def,
---
And pushed now. Thanks for review!
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list