[libvirt] [PATCH 0/3] qemu: Use same create/define overwrite logic for migration
As pointed out by Dan here: https://www.redhat.com/archives/libvir-list/2009-October/msg00784.html We should be using the same logic for collision prevention and domain redefinition in the migrate routines as we use in create/define/restore. Cole Robinson (3): qemu: Remove compiled out localhost migration support qemu: Break out function to check if we can create/define/restore qemu: Use same create/define overwrite logic for migration prepare. src/qemu/qemu_driver.c | 191 ++++++++++++++++-------------------------------- 1 files changed, 62 insertions(+), 129 deletions(-)
Pretty sure this would deadlock now that we have proper locking, so remove the code. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 12 ------------ 1 files changed, 0 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e8606c8..7eed356 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6298,20 +6298,8 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, /* Target domain name, maybe renamed. */ dname = dname ? dname : def->name; -#if 1 /* Ensure the name and UUID don't already exist in an active VM */ vm = virDomainFindByUUID(&driver->domains, def->uuid); -#else - /* For TESTING ONLY you can change #if 1 -> #if 0 above and use - * this code which lets you do localhost migrations. You must still - * supply a fresh 'dname' but this code assigns a random UUID. - */ - if (virUUIDGenerate (def->uuid) == -1) { - qemudReportError (dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("could not generate random UUID")); - goto cleanup; - } -#endif if (!vm) vm = virDomainFindByName(&driver->domains, dname); if (vm) { -- 1.6.5.1
On Mon, Nov 02, 2009 at 02:52:37PM -0500, Cole Robinson wrote:
Pretty sure this would deadlock now that we have proper locking, so remove the code.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 12 ------------ 1 files changed, 0 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e8606c8..7eed356 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6298,20 +6298,8 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, /* Target domain name, maybe renamed. */ dname = dname ? dname : def->name;
-#if 1 /* Ensure the name and UUID don't already exist in an active VM */ vm = virDomainFindByUUID(&driver->domains, def->uuid); -#else - /* For TESTING ONLY you can change #if 1 -> #if 0 above and use - * this code which lets you do localhost migrations. You must still - * supply a fresh 'dname' but this code assigns a random UUID. - */ - if (virUUIDGenerate (def->uuid) == -1) { - qemudReportError (dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("could not generate random UUID")); - goto cleanup; - } -#endif
if (!vm) vm = virDomainFindByName(&driver->domains, dname); if (vm) {
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 11/03/2009 04:00 PM, Daniel P. Berrange wrote:
On Mon, Nov 02, 2009 at 02:52:37PM -0500, Cole Robinson wrote:
Pretty sure this would deadlock now that we have proper locking, so remove the code.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 12 ------------ 1 files changed, 0 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e8606c8..7eed356 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6298,20 +6298,8 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, /* Target domain name, maybe renamed. */ dname = dname ? dname : def->name;
-#if 1 /* Ensure the name and UUID don't already exist in an active VM */ vm = virDomainFindByUUID(&driver->domains, def->uuid); -#else - /* For TESTING ONLY you can change #if 1 -> #if 0 above and use - * this code which lets you do localhost migrations. You must still - * supply a fresh 'dname' but this code assigns a random UUID. - */ - if (virUUIDGenerate (def->uuid) == -1) { - qemudReportError (dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("could not generate random UUID")); - goto cleanup; - } -#endif
if (!vm) vm = virDomainFindByName(&driver->domains, dname); if (vm) {
ACK
Daniel
I've pushed this patch since it is separate from the rest. I'll post a more complete patch with the other driver changes you suggested. Thanks, Cole
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 151 +++++++++++++++++++----------------------------- 1 files changed, 59 insertions(+), 92 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7eed356..e038887 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2594,6 +2594,59 @@ cleanup: return dom; } +static int +qemudDomainCanCreate(virConnectPtr conn, + virDomainDefPtr def, + unsigned int check_active) +{ + struct qemud_driver *driver = conn->privateData; + int ret = 0; + virDomainObjPtr vm = NULL; + + /* See if a VM with matching UUID already exists */ + vm = virDomainFindByUUID(&driver->domains, def->uuid); + if (vm) { + /* UUID matches, but if names don't match, refuse it */ + if (STRNEQ(vm->def->name, def->name)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("domain '%s' is already defined with uuid %s"), + vm->def->name, uuidstr); + goto cleanup; + } + + if (check_active) { + /* UUID & name match, but if VM is already active, refuse it */ + if (virDomainObjIsActive(vm)) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_INVALID, + _("domain is already active as '%s'"), + vm->def->name); + goto cleanup; + } + } + + virDomainObjUnlock(vm); + } else { + /* UUID does not match, but if a name matches, refuse it */ + vm = virDomainFindByName(&driver->domains, def->name); + if (vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("domain '%s' already exists with uuid %s"), + def->name, uuidstr); + goto cleanup; + } + } + + ret = 1; +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +} + static int qemudGetVersion(virConnectPtr conn, unsigned long *version) { struct qemud_driver *driver = conn->privateData; int ret = -1; @@ -2648,38 +2701,8 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (virSecurityDriverVerify(conn, def) < 0) goto cleanup; - /* See if a VM with matching UUID already exists */ - vm = virDomainFindByUUID(&driver->domains, def->uuid); - if (vm) { - /* UUID matches, but if names don't match, refuse it */ - if (STRNEQ(vm->def->name, def->name)) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - vm->def->name, uuidstr); - goto cleanup; - } - - /* UUID & name match, but if VM is already active, refuse it */ - if (virDomainObjIsActive(vm)) { - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain is already active as '%s'"), vm->def->name); - goto cleanup; - } - virDomainObjUnlock(vm); - } else { - /* UUID does not match, but if a name matches, refuse it */ - vm = virDomainFindByName(&driver->domains, def->name); - if (vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - def->name, uuidstr); - goto cleanup; - } - } + if (!qemudDomainCanCreate(conn, def, 1)) + goto cleanup; if (!(vm = virDomainAssignDef(conn, driver->caps, @@ -3709,38 +3732,8 @@ static int qemudDomainRestore(virConnectPtr conn, goto cleanup; } - /* See if a VM with matching UUID already exists */ - vm = virDomainFindByUUID(&driver->domains, def->uuid); - if (vm) { - /* UUID matches, but if names don't match, refuse it */ - if (STRNEQ(vm->def->name, def->name)) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - vm->def->name, uuidstr); - goto cleanup; - } - - /* UUID & name match, but if VM is already active, refuse it */ - if (virDomainObjIsActive(vm)) { - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_INVALID, - _("domain is already active as '%s'"), vm->def->name); - goto cleanup; - } - virDomainObjUnlock(vm); - } else { - /* UUID does not match, but if a name matches, refuse it */ - vm = virDomainFindByName(&driver->domains, def->name); - if (vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - def->name, uuidstr); - goto cleanup; - } - } + if (!qemudDomainCanCreate(conn, def, 1)) + goto cleanup; if (!(vm = virDomainAssignDef(conn, driver->caps, @@ -4197,34 +4190,8 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { if (virSecurityDriverVerify(conn, def) < 0) goto cleanup; - /* See if a VM with matching UUID already exists */ - vm = virDomainFindByUUID(&driver->domains, def->uuid); - if (vm) { - /* UUID matches, but if names don't match, refuse it */ - if (STRNEQ(vm->def->name, def->name)) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - vm->def->name, uuidstr); - goto cleanup; - } - - /* UUID & name match */ - virDomainObjUnlock(vm); - newVM = 0; - } else { - /* UUID does not match, but if a name matches, refuse it */ - vm = virDomainFindByName(&driver->domains, def->name); - if (vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - def->name, uuidstr); - goto cleanup; - } - } + if (!qemudDomainCanCreate(conn, def, 0)) + goto cleanup; if (qemudCanonicalizeMachine(driver, def) < 0) goto cleanup; -- 1.6.5.1
On 11/02/2009 02:52 PM, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 151 +++++++++++++++++++----------------------------- 1 files changed, 59 insertions(+), 92 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7eed356..e038887 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2594,6 +2594,59 @@ cleanup: return dom; }
+static int +qemudDomainCanCreate(virConnectPtr conn, + virDomainDefPtr def, + unsigned int check_active) +{ + struct qemud_driver *driver = conn->privateData; + int ret = 0; + virDomainObjPtr vm = NULL; + + /* See if a VM with matching UUID already exists */ + vm = virDomainFindByUUID(&driver->domains, def->uuid); + if (vm) { + /* UUID matches, but if names don't match, refuse it */ + if (STRNEQ(vm->def->name, def->name)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("domain '%s' is already defined with uuid %s"), + vm->def->name, uuidstr); + goto cleanup; + } + + if (check_active) { + /* UUID & name match, but if VM is already active, refuse it */ + if (virDomainObjIsActive(vm)) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_INVALID, + _("domain is already active as '%s'"), + vm->def->name); + goto cleanup; + } + } + + virDomainObjUnlock(vm); + } else { + /* UUID does not match, but if a name matches, refuse it */ + vm = virDomainFindByName(&driver->domains, def->name); + if (vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("domain '%s' already exists with uuid %s"), + def->name, uuidstr); + goto cleanup; + } + } + + ret = 1; +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +} + static int qemudGetVersion(virConnectPtr conn, unsigned long *version) { struct qemud_driver *driver = conn->privateData; int ret = -1; @@ -2648,38 +2701,8 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (virSecurityDriverVerify(conn, def) < 0) goto cleanup;
- /* See if a VM with matching UUID already exists */ - vm = virDomainFindByUUID(&driver->domains, def->uuid); - if (vm) { - /* UUID matches, but if names don't match, refuse it */ - if (STRNEQ(vm->def->name, def->name)) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - vm->def->name, uuidstr); - goto cleanup; - } - - /* UUID & name match, but if VM is already active, refuse it */ - if (virDomainObjIsActive(vm)) { - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain is already active as '%s'"), vm->def->name); - goto cleanup; - } - virDomainObjUnlock(vm); - } else { - /* UUID does not match, but if a name matches, refuse it */ - vm = virDomainFindByName(&driver->domains, def->name); - if (vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - def->name, uuidstr); - goto cleanup; - } - } + if (!qemudDomainCanCreate(conn, def, 1)) + goto cleanup;
if (!(vm = virDomainAssignDef(conn, driver->caps, @@ -3709,38 +3732,8 @@ static int qemudDomainRestore(virConnectPtr conn, goto cleanup; }
- /* See if a VM with matching UUID already exists */ - vm = virDomainFindByUUID(&driver->domains, def->uuid); - if (vm) { - /* UUID matches, but if names don't match, refuse it */ - if (STRNEQ(vm->def->name, def->name)) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - vm->def->name, uuidstr); - goto cleanup; - } - - /* UUID & name match, but if VM is already active, refuse it */ - if (virDomainObjIsActive(vm)) { - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_INVALID, - _("domain is already active as '%s'"), vm->def->name); - goto cleanup; - } - virDomainObjUnlock(vm); - } else { - /* UUID does not match, but if a name matches, refuse it */ - vm = virDomainFindByName(&driver->domains, def->name); - if (vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - def->name, uuidstr); - goto cleanup; - } - } + if (!qemudDomainCanCreate(conn, def, 1)) + goto cleanup;
if (!(vm = virDomainAssignDef(conn, driver->caps, @@ -4197,34 +4190,8 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { if (virSecurityDriverVerify(conn, def) < 0) goto cleanup;
- /* See if a VM with matching UUID already exists */ - vm = virDomainFindByUUID(&driver->domains, def->uuid); - if (vm) { - /* UUID matches, but if names don't match, refuse it */ - if (STRNEQ(vm->def->name, def->name)) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain '%s' is already defined with uuid %s"), - vm->def->name, uuidstr); - goto cleanup; - } - - /* UUID & name match */ - virDomainObjUnlock(vm); - newVM = 0;
Just noticed that the above line is dropped in this change. I'll send a fixed patch. - Cole
On Mon, Nov 02, 2009 at 02:52:38PM -0500, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 151 +++++++++++++++++++----------------------------- 1 files changed, 59 insertions(+), 92 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7eed356..e038887 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2594,6 +2594,59 @@ cleanup: return dom; }
+static int +qemudDomainCanCreate(virConnectPtr conn, + virDomainDefPtr def, + unsigned int check_active)
How about putting this into src/conf/domain_conf.c - this same logic is needed for LXC and UML drivers too. Something like virDomainObjIsDuplicate(virDomainObjListPtr doms, virDomainDefPtr def, unsigned int check_active); Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 30 ++++-------------------------- 1 files changed, 4 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e038887..6981d99 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6041,19 +6041,8 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, /* Target domain name, maybe renamed. */ dname = dname ? dname : def->name; - /* Ensure the name and UUID don't already exist in an active VM */ - vm = virDomainFindByUUID(&driver->domains, def->uuid); - - if (!vm) vm = virDomainFindByName(&driver->domains, dname); - if (vm) { - if (virDomainObjIsActive(vm)) { - qemudReportError(dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain with the same name or UUID already exists as '%s'"), - vm->def->name); - goto cleanup; - } - virDomainObjUnlock(vm); - } + if (!qemudDomainCanCreate(dconn, def, 1)) + goto cleanup; if (!(vm = virDomainAssignDef(dconn, driver->caps, @@ -6265,19 +6254,8 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, /* Target domain name, maybe renamed. */ dname = dname ? dname : def->name; - /* Ensure the name and UUID don't already exist in an active VM */ - vm = virDomainFindByUUID(&driver->domains, def->uuid); - - if (!vm) vm = virDomainFindByName(&driver->domains, dname); - if (vm) { - if (virDomainObjIsActive(vm)) { - qemudReportError (dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("domain with the same name or UUID already exists as '%s'"), - vm->def->name); - goto cleanup; - } - virDomainObjUnlock(vm); - } + if (!qemudDomainCanCreate(dconn, def, 1)) + goto cleanup; if (!(vm = virDomainAssignDef(dconn, driver->caps, -- 1.6.5.1
On Mon, Nov 02, 2009 at 02:52:36PM -0500, Cole Robinson wrote:
As pointed out by Dan here:
https://www.redhat.com/archives/libvir-list/2009-October/msg00784.html
We should be using the same logic for collision prevention and domain redefinition in the migrate routines as we use in create/define/restore.
Yup, just more racy :-\
Cole Robinson (3): qemu: Remove compiled out localhost migration support qemu: Break out function to check if we can create/define/restore qemu: Use same create/define overwrite logic for migration prepare.
src/qemu/qemu_driver.c | 191 ++++++++++++++++-------------------------------- 1 files changed, 62 insertions(+), 129 deletions(-)
Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard