[libvirt] [PATCH 0/3] qemu: fix startup policy checking
One of the recent refactors broke disk startup policy checking. Fix it with a few cleanups. Peter Krempa (3): qemu: domain: Sanitize return value handling in disk presence checker qemu: process: Unexport qemuProcessStartValidate qemu: process: Call disk startup policy check after cloning domain def src/qemu/qemu_domain.c | 16 +++++----------- src/qemu/qemu_process.c | 14 +++++++++----- src/qemu/qemu_process.h | 7 ------- 3 files changed, 14 insertions(+), 23 deletions(-) -- 2.8.3
One of the functions is returning always 0 and the second one uses unnecessary labels. --- src/qemu/qemu_domain.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c21465d..fe64a55 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3863,7 +3863,7 @@ qemuDomainSetFakeReboot(virQEMUDriverPtr driver, virObjectUnref(cfg); } -static int +static void qemuDomainCheckRemoveOptionalDisk(virQEMUDriverPtr driver, virDomainObjPtr vm, size_t diskIndex) @@ -3895,8 +3895,6 @@ qemuDomainCheckRemoveOptionalDisk(virQEMUDriverPtr driver, } qemuDomainEventQueue(driver, event); - - return 0; } static int @@ -3916,15 +3914,15 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, if (!cold_boot && device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && device != VIR_DOMAIN_DISK_DEVICE_CDROM) - goto error; + return -1; break; case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: - goto error; + return -1; case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: if (cold_boot) - goto error; + return -1; break; case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: @@ -3933,13 +3931,9 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, break; } - if (qemuDomainCheckRemoveOptionalDisk(driver, vm, diskIndex) < 0) - goto error; + qemuDomainCheckRemoveOptionalDisk(driver, vm, diskIndex); return 0; - - error: - return -1; } -- 2.8.3
On 06/02/2016 10:09 AM, Peter Krempa wrote:
One of the functions is returning always 0 and the second one uses unnecessary labels. --- src/qemu/qemu_domain.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
ACK John
--- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_process.h | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e847cd1..a5bb99e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4325,7 +4325,7 @@ qemuProcessStartValidateXML(virDomainObjPtr vm, * start the domain but create a valid qemu command. If some code shouldn't be * executed in this case, make sure to check this flag. */ -int +static int qemuProcessStartValidate(virQEMUDriverPtr driver, virDomainObjPtr vm, virQEMUCapsPtr qemuCaps, diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 61d9f56..13845d7 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -89,13 +89,6 @@ virCommandPtr qemuProcessCreatePretendCmd(virConnectPtr conn, bool standalone, unsigned int flags); -int qemuProcessStartValidate(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virQEMUCapsPtr qemuCaps, - bool migration, - bool snap, - unsigned int flags); - int qemuProcessInit(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, -- 2.8.3
On 06/02/2016 10:09 AM, Peter Krempa wrote:
--- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_process.h | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-)
already sent/pushed as patch 5 of series: http://www.redhat.com/archives/libvir-list/2016-May/msg02000.html John
On Tue, Jun 07, 2016 at 17:09:53 -0400, John Ferlan wrote:
On 06/02/2016 10:09 AM, Peter Krempa wrote:
--- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_process.h | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-)
already sent/pushed as patch 5 of series:
I've just noticed that while rebasing. I'm wondering how that happened as I don't remember cherry-picking the commit to any branch. I either re-implemented it or messed up a rebase. Sorry for wasting cycles. Peter
In commit 1e38ef72 the disk startup policy check was moved prior to the call to virDomainObjSetDefTransient which dropped the disk from the config rather than the def to be started which is a bug. Additionally we'd not report the disk change event for this since the disk aliases were not set at that point. Finally 'volume' based disks would not work with startup policy too. Fix it by moving it back after the definition is copied, aliases are assigned and disk sources are translated. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1341415 --- src/qemu/qemu_process.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a5bb99e..ad04a96 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4348,10 +4348,6 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, } } - if (qemuDomainCheckDiskPresence(driver, vm, - flags & VIR_QEMU_PROCESS_START_COLD) < 0) - return -1; - VIR_DEBUG("Checking domain and device security labels"); if (virSecurityManagerCheckAllLabel(driver->securityManager, vm->def) < 0) return -1; @@ -4876,6 +4872,14 @@ qemuProcessPrepareDomain(virConnectPtr conn, goto cleanup; } + /* drop possibly missing disks from the definition. This needs to happen + * after the def is copied, aliases are set and disk sources are translated */ + if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { + if (qemuDomainCheckDiskPresence(driver, vm, + flags & VIR_QEMU_PROCESS_START_COLD) < 0) + goto cleanup; + } + VIR_DEBUG("Create domain masterKey"); if (qemuDomainMasterKeyCreate(vm) < 0) goto cleanup; -- 2.8.3
On 06/02/2016 10:09 AM, Peter Krempa wrote:
In commit 1e38ef72 the disk startup policy check was moved prior to the call to virDomainObjSetDefTransient which dropped the disk from the config rather than the def to be started which is a bug.
Additionally we'd not report the disk change event for this since the disk aliases were not set at that point.
Finally 'volume' based disks would not work with startup policy too.
Fix it by moving it back after the definition is copied, aliases are assigned and disk sources are translated.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1341415 --- src/qemu/qemu_process.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
Does it make sense to check all disk labels for something that's going to be dropped? (e.g. call to virSecurityManagerCheckAllLabel also goes through ndisks)... Your comment below makes me wonder if there's anything else that got moved that may have assumed Translate had been run. Would make that virDomainDiskDefValidate less interesting if the Translate could be done during Init prior to Validate. My head/eyes hurt ACK to what's here though - just was trying to think out loud... John
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a5bb99e..ad04a96 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4348,10 +4348,6 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, } }
- if (qemuDomainCheckDiskPresence(driver, vm, - flags & VIR_QEMU_PROCESS_START_COLD) < 0) - return -1; - VIR_DEBUG("Checking domain and device security labels"); if (virSecurityManagerCheckAllLabel(driver->securityManager, vm->def) < 0) return -1; @@ -4876,6 +4872,14 @@ qemuProcessPrepareDomain(virConnectPtr conn, goto cleanup; }
+ /* drop possibly missing disks from the definition. This needs to happen + * after the def is copied, aliases are set and disk sources are translated */ + if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { + if (qemuDomainCheckDiskPresence(driver, vm, + flags & VIR_QEMU_PROCESS_START_COLD) < 0) + goto cleanup; + } + VIR_DEBUG("Create domain masterKey"); if (qemuDomainMasterKeyCreate(vm) < 0) goto cleanup;
participants (2)
-
John Ferlan -
Peter Krempa