[libvirt] [PATCH 0/7] qemu: refactor disk startup handling (blockdev-add saga)
This is a collection of patches which refactor and fix disk startup code which are part of my work on the blockdev-add feature. Peter Krempa (7): qemu: process: document parameters for startup preparing functions qemu: migration: Extract flags for starting VM into a variable qemu: process: Pass flags to qemuProcessPrepareHost qemu: domain: Document and export qemuDomainCheckDiskStartupPolicy qemu: process: Move 'volume' translation to domain prepare stage qemu: process: Move TLS setup for storage source to qemuProcessPrepareDomainStorage qemu: process: move disk presence checking to host setup function src/qemu/qemu_domain.c | 90 ++++++++---------------------------------- src/qemu/qemu_domain.h | 13 +++---- src/qemu/qemu_migration.c | 12 +++--- src/qemu/qemu_process.c | 99 ++++++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_process.h | 2 +- 5 files changed, 117 insertions(+), 99 deletions(-) -- 2.14.1
Document mainly what flag values are passed in. --- src/qemu/qemu_process.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bde3ba462..7f0ef2664 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5276,7 +5276,11 @@ qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm, /** - * qemuProcessPrepareDomain + * qemuProcessPrepareDomain: + * @conn: connection object (for looking up storage volumes) + * @driver: qemu driver + * @vm: domain object + * @flags: qemuProcessStartFlags * * This function groups all code that modifies only live XML of a domain which * is about to start and it's the only place to do those modifications. @@ -5404,7 +5408,10 @@ qemuProcessPrepareDomain(virConnectPtr conn, /** - * qemuProcessPrepareHost + * qemuProcessPrepareHost: + * @driver: qemu driver + * @vm: domain object + * @incoming: true if we are preparing an incomming migration * * This function groups all code that modifies host system (which also may * update live XML) to prepare environment for a domain which is about to start -- 2.14.1
On 10/04/2017 07:42 AM, Peter Krempa wrote:
Document mainly what flag values are passed in. --- src/qemu/qemu_process.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bde3ba462..7f0ef2664 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5276,7 +5276,11 @@ qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm,
/** - * qemuProcessPrepareDomain + * qemuProcessPrepareDomain: + * @conn: connection object (for looking up storage volumes) + * @driver: qemu driver + * @vm: domain object + * @flags: qemuProcessStartFlags * * This function groups all code that modifies only live XML of a domain which * is about to start and it's the only place to do those modifications. @@ -5404,7 +5408,10 @@ qemuProcessPrepareDomain(virConnectPtr conn,
/** - * qemuProcessPrepareHost + * qemuProcessPrepareHost: + * @driver: qemu driver + * @vm: domain object + * @incoming: true if we are preparing an incomming migration
s/incomming/incoming Reviewed-by: John Ferlan <jferlan@redhat.com> John
* * This function groups all code that modifies host system (which also may * update live XML) to prepare environment for a domain which is about to start
qemuMigrationPrepareAny called multiple of the functions starting the qemu process for incomming migration by adding the flags explicitly. Extract them to a variable so that they can be easily used for other calls or changed in the future. --- src/qemu/qemu_migration.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 32904b7b9..078da1dfd 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2511,6 +2511,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, bool tunnel = !!st; char *xmlout = NULL; unsigned int cookieFlags; + unsigned int startFlags; virCapsPtr caps = NULL; qemuProcessIncomingDefPtr incoming = NULL; bool taint_hook = false; @@ -2671,8 +2672,10 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, goto stopjob; } + startFlags = VIR_QEMU_PROCESS_START_AUTODESTROY; + if (qemuProcessInit(driver, vm, mig->cpu, QEMU_ASYNC_JOB_MIGRATION_IN, - true, VIR_QEMU_PROCESS_START_AUTODESTROY) < 0) + true, startFlags) < 0) goto stopjob; stopProcess = true; @@ -2681,8 +2684,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, dataFD[0]))) goto stopjob; - if (qemuProcessPrepareDomain(dconn, driver, vm, - VIR_QEMU_PROCESS_START_AUTODESTROY) < 0) + if (qemuProcessPrepareDomain(dconn, driver, vm, startFlags) < 0) goto stopjob; if (qemuProcessPrepareHost(driver, vm, !!incoming) < 0) @@ -2691,7 +2693,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, rv = qemuProcessLaunch(dconn, driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN, incoming, NULL, VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START, - VIR_QEMU_PROCESS_START_AUTODESTROY); + startFlags); if (rv < 0) { if (rv == -2) relabel = true; -- 2.14.1
On 10/04/2017 07:42 AM, Peter Krempa wrote:
qemuMigrationPrepareAny called multiple of the functions starting the qemu process for incomming migration by adding the flags explicitly.
incoming
Extract them to a variable so that they can be easily used for other calls or changed in the future. --- src/qemu/qemu_migration.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Pass flags to the function rather than just whether we have incoming migration. This also enforces correct startup policy for USB devices when reverting from a snapshot. --- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 8 ++++---- src/qemu/qemu_process.h | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 078da1dfd..dd60071bf 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2687,7 +2687,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (qemuProcessPrepareDomain(dconn, driver, vm, startFlags) < 0) goto stopjob; - if (qemuProcessPrepareHost(driver, vm, !!incoming) < 0) + if (qemuProcessPrepareHost(driver, vm, startFlags) < 0) goto stopjob; rv = qemuProcessLaunch(dconn, driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7f0ef2664..dfaacbcb9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5411,7 +5411,7 @@ qemuProcessPrepareDomain(virConnectPtr conn, * qemuProcessPrepareHost: * @driver: qemu driver * @vm: domain object - * @incoming: true if we are preparing an incomming migration + * @flags: qemuProcessStartFlags * * This function groups all code that modifies host system (which also may * update live XML) to prepare environment for a domain which is about to start @@ -5422,7 +5422,7 @@ qemuProcessPrepareDomain(virConnectPtr conn, int qemuProcessPrepareHost(virQEMUDriverPtr driver, virDomainObjPtr vm, - bool incoming) + unsigned int flags) { int ret = -1; unsigned int hostdev_flags = 0; @@ -5444,7 +5444,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, VIR_DEBUG("Preparing host devices"); if (!cfg->relaxedACS) hostdev_flags |= VIR_HOSTDEV_STRICT_ACS_CHECK; - if (!incoming) + if (flags & VIR_QEMU_PROCESS_START_NEW) hostdev_flags |= VIR_HOSTDEV_COLD_BOOT; if (qemuHostdevPrepareDomainDevices(driver, vm->def, priv->qemuCaps, hostdev_flags) < 0) @@ -5960,7 +5960,7 @@ qemuProcessStart(virConnectPtr conn, if (qemuProcessPrepareDomain(conn, driver, vm, flags) < 0) goto stop; - if (qemuProcessPrepareHost(driver, vm, !!incoming) < 0) + if (qemuProcessPrepareHost(driver, vm, flags) < 0) goto stop; if ((rv = qemuProcessLaunch(conn, driver, vm, asyncJob, incoming, diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 667d5c53d..814b86d8a 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -111,7 +111,7 @@ int qemuProcessPrepareDomain(virConnectPtr conn, int qemuProcessPrepareHost(virQEMUDriverPtr driver, virDomainObjPtr vm, - bool incoming); + unsigned int flags); int qemuProcessLaunch(virConnectPtr conn, virQEMUDriverPtr driver, -- 2.14.1
On 10/04/2017 07:42 AM, Peter Krempa wrote:
Pass flags to the function rather than just whether we have incoming migration. This also enforces correct startup policy for USB devices when reverting from a snapshot. --- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 8 ++++---- src/qemu/qemu_process.h | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 078da1dfd..dd60071bf 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2687,7 +2687,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (qemuProcessPrepareDomain(dconn, driver, vm, startFlags) < 0) goto stopjob;
- if (qemuProcessPrepareHost(driver, vm, !!incoming) < 0) + if (qemuProcessPrepareHost(driver, vm, startFlags) < 0)
yah! - removing an essentially useless !!incoming since it couldn't be anything but true... Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
On Wed, Oct 04, 2017 at 11:59:07 -0400, John Ferlan wrote:
On 10/04/2017 07:42 AM, Peter Krempa wrote:
Pass flags to the function rather than just whether we have incoming migration. This also enforces correct startup policy for USB devices when reverting from a snapshot. --- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 8 ++++---- src/qemu/qemu_process.h | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 078da1dfd..dd60071bf 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2687,7 +2687,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (qemuProcessPrepareDomain(dconn, driver, vm, startFlags) < 0) goto stopjob;
- if (qemuProcessPrepareHost(driver, vm, !!incoming) < 0) + if (qemuProcessPrepareHost(driver, vm, startFlags) < 0)
yah! - removing an essentially useless !!incoming since it couldn't be anything but true...
Well. Now it won't be anything but VIR_QEMU_PROCESS_START_AUTODESTROY
--- src/qemu/qemu_domain.c | 18 +++++++++++++++++- src/qemu/qemu_domain.h | 5 +++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b202d02f9..8aa082618 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5625,7 +5625,23 @@ qemuDomainCheckRemoveOptionalDisk(virQEMUDriverPtr driver, qemuDomainEventQueue(driver, event); } -static int + +/** + * qemuDomainCheckDiskStartupPolicy: + * @driver: qemu driver object + * @vm: domain object + * @disk: index of disk to check + * @cold_boot: true if a new VM is being started + * + * This function should be called when the source storage for a disk device is + * missing. The function checks whether the startup policy for the disk allows + * removal of the source (or disk) according to the state of the VM. + * + * The function returns 0 if the source or disk was dropped and -1 if the state + * of the VM does not allow this. This function does not report errors, but + * clears any reported error if 0 is returned. + */ +int qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, virDomainObjPtr vm, size_t diskIndex, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 1a47396ab..b3db50c2f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -639,6 +639,11 @@ void qemuDomainSetFakeReboot(virQEMUDriverPtr driver, bool qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job); +int qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, + virDomainObjPtr vm, + size_t diskIndex, + bool cold_boot); + int qemuDomainCheckDiskPresence(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, -- 2.14.1
On 10/04/2017 07:42 AM, Peter Krempa wrote:
--- src/qemu/qemu_domain.c | 18 +++++++++++++++++- src/qemu/qemu_domain.h | 5 +++++ 2 files changed, 22 insertions(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Introduce a new function to prepare domain disks which will also do the volume source to actual disk source translation. --- src/qemu/qemu_domain.c | 10 +--------- src/qemu/qemu_domain.h | 3 +-- src/qemu/qemu_process.c | 36 ++++++++++++++++++++++++++++++++---- 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8aa082618..bf2ce29bf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5682,8 +5682,7 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, int -qemuDomainCheckDiskPresence(virConnectPtr conn, - virQEMUDriverPtr driver, +qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int flags) { @@ -5697,13 +5696,6 @@ qemuDomainCheckDiskPresence(virConnectPtr conn, virDomainDiskDefPtr disk = vm->def->disks[idx]; virStorageFileFormat format = virDomainDiskGetFormat(disk); - if (virStorageTranslateDiskSourcePool(conn, vm->def->disks[idx]) < 0) { - if (pretend || - qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) < 0) - return -1; - continue; - } - if (pretend) continue; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b3db50c2f..914f2bec9 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -644,8 +644,7 @@ int qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, size_t diskIndex, bool cold_boot); -int qemuDomainCheckDiskPresence(virConnectPtr conn, - virQEMUDriverPtr driver, +int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int flags); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index dfaacbcb9..ad7c7ee81 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5275,6 +5275,32 @@ qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm, } +static int +qemuProcessPrepareDomainStorage(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + size_t i; + bool cold_boot = flags & VIR_QEMU_PROCESS_START_COLD; + + for (i = vm->def->ndisks; i > 0; i--) { + size_t idx = i - 1; + virDomainDiskDefPtr disk = vm->def->disks[idx]; + + if (virStorageTranslateDiskSourcePool(conn, disk) < 0) { + if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) < 0) + return -1; + + /* disk source was dropped */ + continue; + } + } + + return 0; +} + + /** * qemuProcessPrepareDomain: * @conn: connection object (for looking up storage volumes) @@ -5351,10 +5377,12 @@ qemuProcessPrepareDomain(virConnectPtr conn, if (qemuProcessSetupGraphics(driver, vm, flags) < 0) goto cleanup; - /* Drop possibly missing disks from the definition. This function - * also resolves source pool/volume into a path and it needs to - * happen after the def is copied and aliases are set. */ - if (qemuDomainCheckDiskPresence(conn, driver, vm, flags) < 0) + VIR_DEBUG("Setting up storage"); + if (qemuProcessPrepareDomainStorage(conn, driver, vm, flags) < 0) + goto cleanup; + + /* Drop possibly missing disks from the definition. */ + if (qemuDomainCheckDiskPresence(driver, vm, flags) < 0) goto cleanup; VIR_DEBUG("Create domain masterKey"); -- 2.14.1
On 10/04/2017 07:42 AM, Peter Krempa wrote:
Introduce a new function to prepare domain disks which will also do the volume source to actual disk source translation. --- src/qemu/qemu_domain.c | 10 +--------- src/qemu/qemu_domain.h | 3 +-- src/qemu/qemu_process.c | 36 ++++++++++++++++++++++++++++++++---- 3 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8aa082618..bf2ce29bf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5682,8 +5682,7 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
int -qemuDomainCheckDiskPresence(virConnectPtr conn, - virQEMUDriverPtr driver, +qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int flags) { @@ -5697,13 +5696,6 @@ qemuDomainCheckDiskPresence(virConnectPtr conn, virDomainDiskDefPtr disk = vm->def->disks[idx]; virStorageFileFormat format = virDomainDiskGetFormat(disk);
- if (virStorageTranslateDiskSourcePool(conn, vm->def->disks[idx]) < 0) { - if (pretend || - qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) < 0) - return -1; - continue; - } - if (pretend) continue;
So the reality is we don't even have to run through any of the loop if pretend == true as we're literally doing nothing with it. I know, moot point since 2 patches later the whole function moves to being called from PrepareHost which never checked pretend anyway - although it perhaps could now that PrepareHost takes @flags
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b3db50c2f..914f2bec9 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -644,8 +644,7 @@ int qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, size_t diskIndex, bool cold_boot);
-int qemuDomainCheckDiskPresence(virConnectPtr conn, - virQEMUDriverPtr driver, +int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int flags);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index dfaacbcb9..ad7c7ee81 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5275,6 +5275,32 @@ qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm, }
+static int +qemuProcessPrepareDomainStorage(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + size_t i; + bool cold_boot = flags & VIR_QEMU_PROCESS_START_COLD; + + for (i = vm->def->ndisks; i > 0; i--) { + size_t idx = i - 1; + virDomainDiskDefPtr disk = vm->def->disks[idx]; + + if (virStorageTranslateDiskSourcePool(conn, disk) < 0) { + if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) < 0)
Missing the "pretend ||" failure option/shortcut from commit 'a2b97a8d' I would think it would be necessary in this failure path since it's still eventually going to be called via/through PrepareDomain which seems to care about pretending... Although make check passes, so who knows.
+ return -1; + + /* disk source was dropped */ + continue; + } + } + + return 0; +} + Reviewed-by: John Ferlan <jferlan@redhat.com>
On Wed, Oct 04, 2017 at 12:06:28 -0400, John Ferlan wrote:
On 10/04/2017 07:42 AM, Peter Krempa wrote:
Introduce a new function to prepare domain disks which will also do the volume source to actual disk source translation. --- src/qemu/qemu_domain.c | 10 +--------- src/qemu/qemu_domain.h | 3 +-- src/qemu/qemu_process.c | 36 ++++++++++++++++++++++++++++++++---- 3 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8aa082618..bf2ce29bf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5682,8 +5682,7 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
int -qemuDomainCheckDiskPresence(virConnectPtr conn, - virQEMUDriverPtr driver, +qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int flags) { @@ -5697,13 +5696,6 @@ qemuDomainCheckDiskPresence(virConnectPtr conn, virDomainDiskDefPtr disk = vm->def->disks[idx]; virStorageFileFormat format = virDomainDiskGetFormat(disk);
- if (virStorageTranslateDiskSourcePool(conn, vm->def->disks[idx]) < 0) { - if (pretend || - qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) < 0) - return -1; - continue; - } - if (pretend) continue;
So the reality is we don't even have to run through any of the loop if pretend == true as we're literally doing nothing with it.
I know, moot point since 2 patches later the whole function moves to being called from PrepareHost which never checked pretend anyway - although it perhaps could now that PrepareHost takes @flags
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b3db50c2f..914f2bec9 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -644,8 +644,7 @@ int qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, size_t diskIndex, bool cold_boot);
-int qemuDomainCheckDiskPresence(virConnectPtr conn, - virQEMUDriverPtr driver, +int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int flags);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index dfaacbcb9..ad7c7ee81 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5275,6 +5275,32 @@ qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm, }
+static int +qemuProcessPrepareDomainStorage(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + size_t i; + bool cold_boot = flags & VIR_QEMU_PROCESS_START_COLD; + + for (i = vm->def->ndisks; i > 0; i--) { + size_t idx = i - 1; + virDomainDiskDefPtr disk = vm->def->disks[idx]; + + if (virStorageTranslateDiskSourcePool(conn, disk) < 0) { + if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) < 0)
Missing the "pretend ||" failure option/shortcut from commit 'a2b97a8d'
I would think it would be necessary in this failure path since it's still eventually going to be called via/through PrepareDomain which seems to care about pretending... Although make check passes, so who knows.
The condition skips dropping of the disk from the definition. Which actually makes it hard to test whether the dropping code works (bud necessitates setup of the volume to test it). Since neither of the tests were present in the testsuite I think we should not special case it. If a test wants to test that volume lookup works properly, it will need to set them up. Otherwise the command line generator will not work. I think of the condition as a bug in the testsuite. I'll document that we are going to drop it.
+ return -1; + + /* disk source was dropped */ + continue; + } + } + + return 0; +} + Reviewed-by: John Ferlan <jferlan@redhat.com>
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
--- src/qemu/qemu_domain.c | 23 ----------------------- src/qemu/qemu_domain.h | 5 ----- src/qemu/qemu_process.c | 10 +++++----- 3 files changed, 5 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bf2ce29bf..d3d5dbac6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7720,29 +7720,6 @@ qemuDomainPrepareDiskSourceTLS(virStorageSourcePtr src, } -/* qemuProcessPrepareDiskSource: - * @def: live domain definition - * @driver: qemu driver - * - * Returns 0 on success, -1 on failure - */ -int -qemuDomainPrepareDiskSource(virDomainDefPtr def, - virQEMUDriverConfigPtr cfg) -{ - size_t i; - - for (i = 0; i < def->ndisks; i++) { - if (qemuDomainPrepareDiskSourceTLS(def->disks[i]->src, - def->disks[i]->info.alias, - cfg) < 0) - return -1; - } - - return 0; -} - - int qemuDomainPrepareShmemChardev(virDomainShmemDefPtr shmem) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 914f2bec9..01e8d629e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -873,11 +873,6 @@ qemuDomainPrepareDiskSourceTLS(virStorageSourcePtr src, virQEMUDriverConfigPtr cfg) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); -int -qemuDomainPrepareDiskSource(virDomainDefPtr def, - virQEMUDriverConfigPtr cfg) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - int qemuDomainPrepareShmemChardev(virDomainShmemDefPtr shmem) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ad7c7ee81..84792c2a7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5279,6 +5279,7 @@ static int qemuProcessPrepareDomainStorage(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, + virQEMUDriverConfigPtr cfg, unsigned int flags) { size_t i; @@ -5295,6 +5296,9 @@ qemuProcessPrepareDomainStorage(virConnectPtr conn, /* disk source was dropped */ continue; } + + if (qemuDomainPrepareDiskSourceTLS(disk->src, disk->info.alias, cfg) < 0) + return -1; } return 0; @@ -5378,7 +5382,7 @@ qemuProcessPrepareDomain(virConnectPtr conn, goto cleanup; VIR_DEBUG("Setting up storage"); - if (qemuProcessPrepareDomainStorage(conn, driver, vm, flags) < 0) + if (qemuProcessPrepareDomainStorage(conn, driver, vm, cfg, flags) < 0) goto cleanup; /* Drop possibly missing disks from the definition. */ @@ -5389,10 +5393,6 @@ qemuProcessPrepareDomain(virConnectPtr conn, if (qemuDomainMasterKeyCreate(vm) < 0) goto cleanup; - VIR_DEBUG("Prepare disk source backends for TLS"); - if (qemuDomainPrepareDiskSource(vm->def, cfg) < 0) - goto cleanup; - VIR_DEBUG("Prepare chardev source backends for TLS"); qemuDomainPrepareChardevSource(vm->def, cfg); -- 2.14.1
On 10/04/2017 07:42 AM, Peter Krempa wrote:
--- src/qemu/qemu_domain.c | 23 ----------------------- src/qemu/qemu_domain.h | 5 ----- src/qemu/qemu_process.c | 10 +++++----- 3 files changed, 5 insertions(+), 33 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Checking of disk presence accesses storage on the host so it should be done from the host setup function. Move the code to new function called qemuProcessPrepareHostStorage and remove qemuDomainCheckDiskPresence. --- src/qemu/qemu_domain.c | 41 ----------------------------------------- src/qemu/qemu_process.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 45 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d3d5dbac6..a8c718f62 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5681,47 +5681,6 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, } -int -qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, - virDomainObjPtr vm, - unsigned int flags) -{ - size_t i; - bool pretend = flags & VIR_QEMU_PROCESS_START_PRETEND; - bool cold_boot = flags & VIR_QEMU_PROCESS_START_COLD; - - VIR_DEBUG("Checking for disk presence"); - for (i = vm->def->ndisks; i > 0; i--) { - size_t idx = i - 1; - virDomainDiskDefPtr disk = vm->def->disks[idx]; - virStorageFileFormat format = virDomainDiskGetFormat(disk); - - if (pretend) - continue; - - if (virStorageSourceIsEmpty(disk->src)) - continue; - - /* There is no need to check the backing chain for disks - * without backing support, the fact that the file exists is - * more than enough */ - if (virStorageSourceIsLocalStorage(disk->src) && - format > VIR_STORAGE_FILE_NONE && - format < VIR_STORAGE_FILE_BACKING && - virFileExists(virDomainDiskGetSource(disk))) - continue; - - if (qemuDomainDetermineDiskChain(driver, vm, disk, true, true) >= 0) - continue; - - if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0) - continue; - - return -1; - } - - return 0; -} /* * The vm must be locked when any of the following cleanup functions is diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 84792c2a7..6bebfe4f4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5385,10 +5385,6 @@ qemuProcessPrepareDomain(virConnectPtr conn, if (qemuProcessPrepareDomainStorage(conn, driver, vm, cfg, flags) < 0) goto cleanup; - /* Drop possibly missing disks from the definition. */ - if (qemuDomainCheckDiskPresence(driver, vm, flags) < 0) - goto cleanup; - VIR_DEBUG("Create domain masterKey"); if (qemuDomainMasterKeyCreate(vm) < 0) goto cleanup; @@ -5435,6 +5431,44 @@ qemuProcessPrepareDomain(virConnectPtr conn, } +static int +qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + size_t i; + bool cold_boot = flags & VIR_QEMU_PROCESS_START_COLD; + + for (i = vm->def->ndisks; i > 0; i--) { + size_t idx = i - 1; + virDomainDiskDefPtr disk = vm->def->disks[idx]; + virStorageFileFormat format = virDomainDiskGetFormat(disk); + + if (virStorageSourceIsEmpty(disk->src)) + continue; + + /* There is no need to check the backing chain for disks + * without backing support, the fact that the file exists is + * more than enough */ + if (virStorageSourceIsLocalStorage(disk->src) && + format > VIR_STORAGE_FILE_NONE && + format < VIR_STORAGE_FILE_BACKING && + virFileExists(virDomainDiskGetSource(disk))) + continue; + + if (qemuDomainDetermineDiskChain(driver, vm, disk, true, true) >= 0) + continue; + + if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0) + continue; + + return -1; + } + + return 0; +} + + /** * qemuProcessPrepareHost: * @driver: qemu driver @@ -5527,6 +5561,10 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, if (qemuDomainWriteMasterKeyFile(driver, vm) < 0) goto cleanup; + VIR_DEBUG("Preparing disks (host)"); + if (qemuProcessPrepareHostStorage(driver, vm, flags) < 0) + goto cleanup; + ret = 0; cleanup: virObjectUnref(cfg); -- 2.14.1
On 10/04/2017 07:42 AM, Peter Krempa wrote:
Checking of disk presence accesses storage on the host so it should be done from the host setup function. Move the code to new function called qemuProcessPrepareHostStorage and remove qemuDomainCheckDiskPresence. --- src/qemu/qemu_domain.c | 41 ----------------------------------------- src/qemu/qemu_process.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 45 deletions(-)
[...]
+static int +qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + size_t i; + bool cold_boot = flags & VIR_QEMU_PROCESS_START_COLD; +
Consider my note from patch 5/7. Previously this loop essentially wasn't run for the pretend environment. Now it will be, although since @flags is passed it conceivably could be checked. My make check passes either way, so it may not matter, but figured to point it out for purely testing purposes it seems. Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
On Wed, Oct 04, 2017 at 12:09:04 -0400, John Ferlan wrote:
On 10/04/2017 07:42 AM, Peter Krempa wrote:
Checking of disk presence accesses storage on the host so it should be done from the host setup function. Move the code to new function called qemuProcessPrepareHostStorage and remove qemuDomainCheckDiskPresence. --- src/qemu/qemu_domain.c | 41 ----------------------------------------- src/qemu/qemu_process.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 45 deletions(-)
[...]
+static int +qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + size_t i; + bool cold_boot = flags & VIR_QEMU_PROCESS_START_COLD; +
Consider my note from patch 5/7. Previously this loop essentially wasn't run for the pretend environment. Now it will be, although since @flags is passed it conceivably could be checked. My make check passes either way, so it may not matter, but figured to point it out for purely testing purposes it seems.
It won't. Previously it was called from the domain prepare function which is called in qemuProcessCreatePretendCmd which is the only place that sets VIR_QEMU_PROCESS_START_PRETEND. By moving the code to the "host prepare" function we gain that by default.
On Wed, Oct 04, 2017 at 18:21:24 +0200, Peter Krempa wrote:
On Wed, Oct 04, 2017 at 12:09:04 -0400, John Ferlan wrote:
On 10/04/2017 07:42 AM, Peter Krempa wrote:
Checking of disk presence accesses storage on the host so it should be done from the host setup function. Move the code to new function called qemuProcessPrepareHostStorage and remove qemuDomainCheckDiskPresence. --- src/qemu/qemu_domain.c | 41 ----------------------------------------- src/qemu/qemu_process.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 45 deletions(-)
[...]
+static int +qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + size_t i; + bool cold_boot = flags & VIR_QEMU_PROCESS_START_COLD; +
Consider my note from patch 5/7. Previously this loop essentially wasn't run for the pretend environment. Now it will be, although since @flags is passed it conceivably could be checked. My make check passes either way, so it may not matter, but figured to point it out for purely testing purposes it seems.
It won't. Previously it was called from the domain prepare function which is called in qemuProcessCreatePretendCmd which is the only place that sets VIR_QEMU_PROCESS_START_PRETEND.
By moving the code to the "host prepare" function we gain that by default.
Forgot to finish thought: We get that by default, since the 'host prepare' function is not called from qemuProcessCreatePretendCmd on purpose. The host prepare function agregates stuff that depends on the host state and thus is invalid to call in the test suite.
participants (2)
-
John Ferlan -
Peter Krempa