[libvirt] [PATCH] qemu: Fix startupPolicy for snapshot-revert
Currently, startupPolicy='requisite' was determining cold boot by migrateForm != NULL. That means, if domain was started up with migrateForm set we didn't require disk source path and allowed it to be dropped. However, on snapshot-revert domain wasn't migrated but according to documentation, requisite should drop disk source as well. --- Related BZ: https://bugzilla.redhat.com/show_bug.cgi?id=798938 src/qemu/qemu_driver.c | 16 +++++++++------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 3 ++- src/qemu/qemu_process.h | 1 + 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ddb381a..b17b52c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1358,7 +1358,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; /* XXXX free the 'vm' we created ? */ - if (qemuProcessStart(conn, driver, vm, NULL, + if (qemuProcessStart(conn, driver, vm, NULL, true, (flags & VIR_DOMAIN_START_PAUSED) != 0, (flags & VIR_DOMAIN_START_AUTODESTROY) != 0, -1, NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_CREATE) < 0) { @@ -4107,8 +4107,9 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, } /* Set the migration source and start it up. */ - ret = qemuProcessStart(conn, driver, vm, "stdio", true, - false, *fd, path, NULL, VIR_NETDEV_VPORT_PROFILE_OP_RESTORE); + ret = qemuProcessStart(conn, driver, vm, "stdio", false, true, + false, *fd, path, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_RESTORE); if (intermediatefd != -1) { if (ret < 0) { @@ -4709,8 +4710,9 @@ qemuDomainObjStart(virConnectPtr conn, } } - ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, - autodestroy, -1, NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_CREATE); + ret = qemuProcessStart(conn, driver, vm, NULL, true, start_paused, + autodestroy, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE); virDomainAuditStart(vm, "booted", ret >= 0); if (ret >= 0) { virDomainEventPtr event = @@ -10788,7 +10790,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virDomainObjAssignDef(vm, config, false); rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - true, false, -1, NULL, snap, + false, true, false, -1, NULL, snap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE); virDomainAuditStart(vm, "from-snapshot", rc >= 0); detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; @@ -10878,7 +10880,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (event) qemuDomainEventQueue(driver, event); rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - paused, false, -1, NULL, NULL, + false, paused, false, -1, NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_CREATE); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 77d40c0..92d046a 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1229,7 +1229,7 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, /* Start the QEMU daemon, with the same command-line arguments plus * -incoming $migrateFrom */ - if (qemuProcessStart(dconn, driver, vm, migrateFrom, true, + if (qemuProcessStart(dconn, driver, vm, migrateFrom, false, true, true, dataFD[0], NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) < 0) { virDomainAuditStart(vm, "migrated", false); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7b99814..502de89 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3072,6 +3072,7 @@ int qemuProcessStart(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, const char *migrateFrom, + bool cold_boot, bool start_paused, bool autodestroy, int stdin_fd, @@ -3227,7 +3228,7 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; VIR_DEBUG("Checking for CDROM and floppy presence"); - if (qemuDomainCheckDiskPresence(driver, vm, migrateFrom != NULL) < 0) + if (qemuDomainCheckDiskPresence(driver, vm, cold_boot) < 0) goto cleanup; VIR_DEBUG("Setting up domain cgroup (if required)"); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2c1d0b5..761db6f 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -48,6 +48,7 @@ int qemuProcessStart(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, const char *migrateFrom, + bool cold_boot, bool start_paused, bool autodestroy, int stdin_fd, -- 1.7.8.5
On 03/07/2012 11:19 AM, Michal Privoznik wrote:
Currently, startupPolicy='requisite' was determining cold boot by migrateForm != NULL. That means, if domain was started up with migrateForm set we didn't require disk source path and allowed
s/migrateForm/migrateFrom/ (twice)
it to be dropped. However, on snapshot-revert domain wasn't migrated but according to documentation, requisite should drop disk source as well. ---
Related BZ: https://bugzilla.redhat.com/show_bug.cgi?id=798938
src/qemu/qemu_driver.c | 16 +++++++++------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 3 ++- src/qemu/qemu_process.h | 1 + 4 files changed, 13 insertions(+), 9 deletions(-)
@@ -4107,8 +4107,9 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, }
/* Set the migration source and start it up. */ - ret = qemuProcessStart(conn, driver, vm, "stdio", true, - false, *fd, path, NULL, VIR_NETDEV_VPORT_PROFILE_OP_RESTORE); + ret = qemuProcessStart(conn, driver, vm, "stdio", false, true, + false, *fd, path, NULL,
Yuck - we're starting to rack up so many bools that it's hard to tell which one is which. This patch can go in as-is, but I'd also like to see a followup patch that refactors things into using an 'unsigned int flags' with an internal enum for bit values (QEMU_START_COLD, QEMU_START_PAUSED, QEMU_START_AUTODESTROY, ...). ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07.03.2012 19:36, Eric Blake wrote:
On 03/07/2012 11:19 AM, Michal Privoznik wrote:
Currently, startupPolicy='requisite' was determining cold boot by migrateForm != NULL. That means, if domain was started up with migrateForm set we didn't require disk source path and allowed
s/migrateForm/migrateFrom/ (twice)
it to be dropped. However, on snapshot-revert domain wasn't migrated but according to documentation, requisite should drop disk source as well. ---
Related BZ: https://bugzilla.redhat.com/show_bug.cgi?id=798938
src/qemu/qemu_driver.c | 16 +++++++++------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 3 ++- src/qemu/qemu_process.h | 1 + 4 files changed, 13 insertions(+), 9 deletions(-)
@@ -4107,8 +4107,9 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, }
/* Set the migration source and start it up. */ - ret = qemuProcessStart(conn, driver, vm, "stdio", true, - false, *fd, path, NULL, VIR_NETDEV_VPORT_PROFILE_OP_RESTORE); + ret = qemuProcessStart(conn, driver, vm, "stdio", false, true, + false, *fd, path, NULL,
Yuck - we're starting to rack up so many bools that it's hard to tell which one is which. This patch can go in as-is, but I'd also like to see a followup patch that refactors things into using an 'unsigned int flags' with an internal enum for bit values (QEMU_START_COLD, QEMU_START_PAUSED, QEMU_START_AUTODESTROY, ...).
ACK.
Thanks pushed. And I'll send patch for what you've described. Michal
Currently, we have 3 boolean arguments we have to pass to qemuProcessStart(). As libvirt grows it is harder and harder to remember them and their position. Therefore we should switch to flags instead. --- src/qemu/qemu_driver.c | 45 +++++++++++++++++++++++++++++---------------- src/qemu/qemu_migration.c | 7 ++++--- src/qemu/qemu_process.c | 21 +++++++++++++-------- src/qemu/qemu_process.h | 12 ++++++++---- 4 files changed, 54 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d2c4544..1aa3a28 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1326,10 +1326,16 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, virDomainPtr dom = NULL; virDomainEventPtr event = NULL; virDomainEventPtr event2 = NULL; + unsigned int start_flags = VIR_QEMU_PROCESS_START_COLD; virCheckFlags(VIR_DOMAIN_START_PAUSED | VIR_DOMAIN_START_AUTODESTROY, NULL); + if (flags & VIR_DOMAIN_START_PAUSED) + start_flags |= VIR_QEMU_PROCESS_START_PAUSED; + if (flags & VIR_DOMAIN_START_AUTODESTROY) + start_flags |= VIR_QEMU_PROCESS_START_AUTODESROY; + qemuDriverLock(driver); if (!(def = virDomainDefParseString(driver->caps, xml, QEMU_EXPECTED_VIRT_TYPES, @@ -1358,10 +1364,9 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; /* XXXX free the 'vm' we created ? */ - if (qemuProcessStart(conn, driver, vm, NULL, true, - (flags & VIR_DOMAIN_START_PAUSED) != 0, - (flags & VIR_DOMAIN_START_AUTODESTROY) != 0, - -1, NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_CREATE) < 0) { + if (qemuProcessStart(conn, driver, vm, NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + start_flags) < 0) { virDomainAuditStart(vm, "booted", false); if (qemuDomainObjEndJob(driver, vm) > 0) qemuDomainRemoveInactive(driver, vm); @@ -4109,9 +4114,9 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, } /* Set the migration source and start it up. */ - ret = qemuProcessStart(conn, driver, vm, "stdio", false, true, - false, *fd, path, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_RESTORE); + ret = qemuProcessStart(conn, driver, vm, "stdio", *fd, path, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, + VIR_QEMU_PROCESS_START_PAUSED); if (intermediatefd != -1) { if (ret < 0) { @@ -4681,6 +4686,10 @@ qemuDomainObjStart(virConnectPtr conn, bool autodestroy = (flags & VIR_DOMAIN_START_AUTODESTROY) != 0; bool bypass_cache = (flags & VIR_DOMAIN_START_BYPASS_CACHE) != 0; bool force_boot = (flags & VIR_DOMAIN_START_FORCE_BOOT) != 0; + unsigned int start_flags = VIR_QEMU_PROCESS_START_COLD; + + start_flags |= start_paused ? VIR_QEMU_PROCESS_START_PAUSED : 0; + start_flags |= autodestroy ? VIR_QEMU_PROCESS_START_AUTODESROY : 0; /* * If there is a managed saved state restore it instead of starting @@ -4712,9 +4721,8 @@ qemuDomainObjStart(virConnectPtr conn, } } - ret = qemuProcessStart(conn, driver, vm, NULL, true, start_paused, - autodestroy, -1, NULL, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE); + ret = qemuProcessStart(conn, driver, vm, NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "booted", ret >= 0); if (ret >= 0) { virDomainEventPtr event = @@ -10795,9 +10803,10 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (config) virDomainObjAssignDef(vm, config, false); - rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - false, true, false, -1, NULL, snap, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE); + rc = qemuProcessStart(snapshot->domain->conn, + driver, vm, NULL, -1, NULL, snap, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + VIR_QEMU_PROCESS_START_PAUSED); virDomainAuditStart(vm, "from-snapshot", rc >= 0); detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; event = virDomainEventNewFromObj(vm, @@ -10882,12 +10891,16 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { /* Flush first event, now do transition 2 or 3 */ bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0; + unsigned int start_flags = 0; + + start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0; if (event) qemuDomainEventQueue(driver, event); - rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - false, paused, false, -1, NULL, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE); + rc = qemuProcessStart(snapshot->domain->conn, + driver, vm, NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) { if (!vm->persistent) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 92d046a..b9b511e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1229,9 +1229,10 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, /* Start the QEMU daemon, with the same command-line arguments plus * -incoming $migrateFrom */ - if (qemuProcessStart(dconn, driver, vm, migrateFrom, false, true, - true, dataFD[0], NULL, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) < 0) { + if (qemuProcessStart(dconn, driver, vm, migrateFrom, dataFD[0], NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START, + VIR_QEMU_PROCESS_START_PAUSED | + VIR_QEMU_PROCESS_START_AUTODESROY) < 0) { virDomainAuditStart(vm, "migrated", false); /* Note that we don't set an error here because qemuProcessStart * should have already done that. diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1ac892f..ff2b0ae 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3072,13 +3072,11 @@ int qemuProcessStart(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, const char *migrateFrom, - bool cold_boot, - bool start_paused, - bool autodestroy, int stdin_fd, const char *stdin_path, virDomainSnapshotObjPtr snapshot, - enum virNetDevVPortProfileOp vmop) + enum virNetDevVPortProfileOp vmop, + unsigned int flags) { int ret; off_t pos = -1; @@ -3091,6 +3089,12 @@ int qemuProcessStart(virConnectPtr conn, unsigned long cur_balloon; int i; + /* Okay, these are just internal flags, + * but doesn't hurt to check */ + virCheckFlags(VIR_QEMU_PROCESS_START_COLD | + VIR_QEMU_PROCESS_START_PAUSED | + VIR_QEMU_PROCESS_START_AUTODESROY, -1); + hookData.conn = conn; hookData.vm = vm; hookData.driver = driver; @@ -3228,7 +3232,8 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; VIR_DEBUG("Checking for CDROM and floppy presence"); - if (qemuDomainCheckDiskPresence(driver, vm, cold_boot) < 0) + if (qemuDomainCheckDiskPresence(driver, vm, + flags & VIR_QEMU_PROCESS_START_COLD) < 0) goto cleanup; VIR_DEBUG("Setting up domain cgroup (if required)"); @@ -3427,7 +3432,7 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Handshake complete, child running"); if (migrateFrom) - start_paused = true; + flags |= VIR_QEMU_PROCESS_START_PAUSED; if (ret == -1) /* The VM failed to start; tear filters before taps */ virDomainConfVMNWFilterTeardown(vm); @@ -3502,7 +3507,7 @@ int qemuProcessStart(virConnectPtr conn, } qemuDomainObjExitMonitorWithDriver(driver, vm); - if (!start_paused) { + if (flags & ~VIR_QEMU_PROCESS_START_PAUSED) { VIR_DEBUG("Starting domain CPUs"); /* Allow the CPUS to start executing */ if (qemuProcessStartCPUs(driver, vm, conn, @@ -3520,7 +3525,7 @@ int qemuProcessStart(virConnectPtr conn, VIR_DOMAIN_PAUSED_USER); } - if (autodestroy && + if (flags & VIR_QEMU_PROCESS_START_AUTODESROY && qemuProcessAutoDestroyAdd(driver, vm, conn) < 0) goto cleanup; diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 761db6f..6d15081 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -44,17 +44,21 @@ void qemuProcessReconnectAll(virConnectPtr conn, struct qemud_driver *driver); int qemuProcessAssignPCIAddresses(virDomainDefPtr def); +typedef enum { + VIR_QEMU_PROCESS_START_COLD = 1 << 0, + VIR_QEMU_PROCESS_START_PAUSED = 1 << 1, + VIR_QEMU_PROCESS_START_AUTODESROY = 1 << 2, +} qemuProcessStartFlags; + int qemuProcessStart(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, const char *migrateFrom, - bool cold_boot, - bool start_paused, - bool autodestroy, int stdin_fd, const char *stdin_path, virDomainSnapshotObjPtr snapshot, - enum virNetDevVPortProfileOp vmop); + enum virNetDevVPortProfileOp vmop, + unsigned int flags); void qemuProcessStop(struct qemud_driver *driver, virDomainObjPtr vm, -- 1.7.8.5
participants (2)
-
Eric Blake -
Michal Privoznik