Re: [libvirt] [PATCH] qemudDomainCreate: also check uuid
On Thu, Jul 31, 2008 at 07:08:33AM -0400, Guido Günther wrote: > On Wed, Jul 30, 2008 at 12:05:56AM -0500, Charles Duffy wrote: > > It appears that this patch was applied (in commit > > 45616162db2d1e807dbe70e60c67cb701cbd06d8) with the virDomainIsActive(vm) > > checks removed from qemudDomainCreate, such that we fail out with > > "domain [...] is already defined and running" even if the domain is only > > defined but not running. > The error message is confusing. I missed to correct that. Shall I send a > patch? I thing doing more than that is simply too confusing for users. No, no problem, Charles was being confused :-) all set now ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemudDomainCreate: also check uuid
On Wed, Jul 30, 2008 at 02:06:29PM -0500, Charles Duffy wrote: > Blerg; the more complex patch I provided was dangerously wrong. > > Just applying the one that corrects the message WORKSFORME. Okay, done, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemudDomainCreate: also check uuid
On Wed, Jul 30, 2008 at 12:05:56AM -0500, Charles Duffy wrote: > It appears that this patch was applied (in commit > 45616162db2d1e807dbe70e60c67cb701cbd06d8) with the virDomainIsActive(vm) > checks removed from qemudDomainCreate, such that we fail out with > "domain [...] is already defined and running" even if the domain is only > defined but not running. The error message is confusing. I missed to correct that. Shall I send a patch? I thing doing more than that is simply too confusing for users. -- Guido -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemudDomainCreate: also check uuid
On Wed, Jul 30, 2008 at 01:49:50PM -0500, Charles Duffy wrote: > Daniel Veillard wrote: > > Are you disagreeing with the message (which your patch doesn't fix) > >or with the semantic of the check (and then why allow to create a domain > >reusing the UUID of another defined but not running domain, I can only > >see confusion or security problems in doing so) > > The act of creating a domain reusing the UUID of another defined but not > running domain is presumably an action taken with intent to rename that > other domain (and thus should undefine the prior domain holding that > UUID in the process... which, admittedly, the given patch didn't > explicitly do). > > Anyhow, two patches attached; one revises the error messages, while the > other improves behavior with the semantics I assumed, to explicitly > undefine any duplicates found during a create; I'm happy with either. We shouldn't have 'create' automatically undefine the duplicate - we have a explicit undefine method apps can already use if they want that to be done, without needing to encode this policy in the create method. I'm fine with us applying the 2nd patch to fix the error message. 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemudDomainCreate: also check uuid
Blerg; the more complex patch I provided was dangerously wrong. Just applying the one that corrects the message WORKSFORME. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemudDomainCreate: also check uuid
Daniel Veillard wrote:
Are you disagreeing with the message (which your patch doesn't fix)
or with the semantic of the check (and then why allow to create a domain
reusing the UUID of another defined but not running domain, I can only
see confusion or security problems in doing so)
The act of creating a domain reusing the UUID of another defined but not
running domain is presumably an action taken with intent to rename that
other domain (and thus should undefine the prior domain holding that
UUID in the process... which, admittedly, the given patch didn't
explicitly do).
Anyhow, two patches attached; one revises the error messages, while the
other improves behavior with the semantics I assumed, to explicitly
undefine any duplicates found during a create; I'm happy with either.
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index b8fd11c..6ed4d8a 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -2014,22 +2014,38 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
vm = virDomainFindByName(driver->domains, def->name);
if (vm) {
-qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
- _("domain '%s' is already defined and running"),
- def->name);
-virDomainDefFree(def);
-return NULL;
+if(virDomainIsActive(vm)) {
+qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+ _("domain '%s' is already defined and running"),
+ def->name);
+virDomainDefFree(def);
+return NULL;
+} else {
+if(virDomainDeleteConfig(conn, vm) < -1) {
+return NULL;
+} else {
+virDomainRemoveInactive(&driver->domains, vm);
+}
+}
}
vm = virDomainFindByUUID(driver->domains, def->uuid);
if (vm) {
-char uuidstr[VIR_UUID_STRING_BUFLEN];
+if(virDomainIsActive(vm)) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
-virUUIDFormat(def->uuid, uuidstr);
-qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
- _("domain with uuid '%s' is already defined and running"),
- uuidstr);
-virDomainDefFree(def);
-return NULL;
+virUUIDFormat(def->uuid, uuidstr);
+qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+ _("domain with uuid '%s' is already defined and running"),
+ uuidstr);
+virDomainDefFree(def);
+return NULL;
+} else {
+if(virDomainDeleteConfig(conn, vm) < -1) {
+return NULL;
+} else {
+virDomainRemoveInactive(&driver->domains, vm);
+}
+}
}
if (!(vm = virDomainAssignDef(conn,
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 9d661d2..3790db8 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -2015,7 +2015,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
vm = virDomainFindByName(driver->domains, def->name);
if (vm) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
- _("domain '%s' is already defined and running"),
+ _("domain '%s' is already defined"),
def->name);
virDomainDefFree(def);
return NULL;
@@ -2026,7 +2026,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
virUUIDFormat(def->uuid, uuidstr);
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
- _("domain with uuid '%s' is already defined and running"),
+ _("domain with uuid '%s' is already defined"),
uuidstr);
virDomainDefFree(def);
return NULL;
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemudDomainCreate: also check uuid
On Wed, Jul 30, 2008 at 12:05:56AM -0500, Charles Duffy wrote: > It appears that this patch was applied (in commit > 45616162db2d1e807dbe70e60c67cb701cbd06d8) with the virDomainIsActive(vm) > checks removed from qemudDomainCreate, such that we fail out with > "domain [...] is already defined and running" even if the domain is only > defined but not running. > > The attached (completely trivial) patch (created against > 301cbb70aa52db2d8c42bc9f9441366385f0a9c4) resolves this. I agree that the message is wrong. But the idea that you can't create a temporary domain if there is one already defined with the same name or UUID seems sound to me. Are you disagreeing with the message (which your patch doesn't fix) or with the semantic of the check (and then why allow to create a domain reusing the UUID of another defined but not running domain, I can only see confusion or security problems in doing so) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemudDomainCreate: also check uuid
It appears that this patch was applied (in commit
45616162db2d1e807dbe70e60c67cb701cbd06d8) with the virDomainIsActive(vm)
checks removed from qemudDomainCreate, such that we fail out with
"domain [...] is already defined and running" even if the domain is only
defined but not running.
The attached (completely trivial) patch (created against
301cbb70aa52db2d8c42bc9f9441366385f0a9c4) resolves this.
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index b8fd11c..a7ddf11 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -2013,7 +2013,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
return NULL;
vm = virDomainFindByName(driver->domains, def->name);
-if (vm) {
+if (vm && virDomainIsActive(vm)) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
_("domain '%s' is already defined and running"),
def->name);
@@ -2021,7 +2021,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
return NULL;
}
vm = virDomainFindByUUID(driver->domains, def->uuid);
-if (vm) {
+if (vm && virDomainIsActive(vm)) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
virUUIDFormat(def->uuid, uuidstr);
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
