11:49 p.m.
Hi,
currently virsh create foo.xml overwrites running domains. In case of
qemu this leaves detached qemu processes around and the domain creation
fails later on being unable to start other domains afterwards - not
nice.
Attached patch checks if we already have a running domain by that name
and in this case refuses to create a new domain from xml by that name.
Probably this check needs to be pushed further upward since this might
affect other hypervisors too, haven't checked that though.
Cheers,
-- Guido
2:33 a.m.
On Thu, Jul 24, 2008 at 12:49:13AM -0400, Guido Günther wrote:
Hi,
currently virsh create foo.xml overwrites running domains. In case of
qemu this leaves detached qemu processes around and the domain creation
fails later on being unable to start other domains afterwards - not
nice.
Attached patch checks if we already have a running domain by that name
and in this case refuses to create a new domain from xml by that name.
okay looks fine to me
Probably this check needs to be pushed further upward since this
might
affect other hypervisors too, haven't checked that though.
Well for Xen that check is delegated to xend, and in general it's better
to keep it that way to avoid potential races. I guess the problem is
more likely for hypervisors where libvirt(d) runs a process to control the
domain and where there is no central checking done.
OpenVZ and LXC should be checked for that, yes.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
4:01 a.m.
On Thu, Jul 24, 2008 at 12:49:13AM -0400, Guido G?nther wrote:
Hi,
currently virsh create foo.xml overwrites running domains. In case of
qemu this leaves detached qemu processes around and the domain creation
fails later on being unable to start other domains afterwards - not
nice.
Attached patch checks if we already have a running domain by that name
and in this case refuses to create a new domain from xml by that name.
Probably this check needs to be pushed further upward since this might
affect other hypervisors too, haven't checked that though.
Actually, in Xen case, XenD itself will do the checking.
[PATCH] qemudDomainCreate: check if domain is already active
otherwise we overwrite a running domain which lets libvirtd disconnect
from the running qemu.
---
src/qemu_driver.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index b84bdf4..0ad72ae 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -1978,6 +1978,15 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const
char *xml,
if (!(def = virDomainDefParseString(conn, driver->caps, xml)))
return NULL;
+ vm = virDomainFindByName(driver->domains, def->name);
You need to check for UUID clash too.
Regards,
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 :|
9:41 a.m.
On Thu, Jul 24, 2008 at 10:01:32AM +0100, Daniel P. Berrange wrote:
> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
> index b84bdf4..0ad72ae 100644
> --- a/src/qemu_driver.c
> +++ b/src/qemu_driver.c
> @@ -1978,6 +1978,15 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn,
const char *xml,
> if (!(def = virDomainDefParseString(conn, driver->caps, xml)))
> return NULL;
>
> + vm = virDomainFindByName(driver->domains, def->name);
You need to check for UUID clash too.
Indeed. But before fixing this I wonder what
the exact semantics of
domainCreateLinux are. Is it correct that we don't call
virDomainSaveConfiguration?
If so we probably don't even need to check if the domain is running. The
mere existance of a domain with the same name/uuid should probably throw
an error already since having another domain by the same name/uuid known
to libvirt already can be a source of confusion for the user.
-- Guido
10:12 a.m.
On Thu, Jul 24, 2008 at 10:41:31AM -0400, Guido G?nther wrote:
On Thu, Jul 24, 2008 at 10:01:32AM +0100, Daniel P. Berrange wrote:
> > diff --git a/src/qemu_driver.c b/src/qemu_driver.c
> > index b84bdf4..0ad72ae 100644
> > --- a/src/qemu_driver.c
> > +++ b/src/qemu_driver.c
> > @@ -1978,6 +1978,15 @@ static virDomainPtr qemudDomainCreate(virConnectPtr
conn, const char *xml,
> > if (!(def = virDomainDefParseString(conn, driver->caps, xml)))
> > return NULL;
> >
> > + vm = virDomainFindByName(driver->domains, def->name);
>
> You need to check for UUID clash too.
Indeed. But before fixing this I wonder what the exact semantics of
domainCreateLinux are. Is it correct that we don't call
virDomainSaveConfiguration?
Yes, that is correct.
virDomainCreateLinux() starts a virtual machine with no config file. All
trace of it will disappear when it shuts down - a so called 'transient'
VM.
Alternatively you can define the config first with virDomainDefineXML()
and then start it based on this definition with virDomainCreate(). This
gives you a persistent VM.
Now, while a transient VM is running you can explicitly give it a config
file by called virDomainDefineXML with the same uuid, thus turning it in
to a persistent VM.
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 :|
2:45 p.m.
New subject: [libvirt] [PATCH] qemudDomainCreate: also check uuid
On Thu, Jul 24, 2008 at 04:12:05PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 24, 2008 at 10:41:31AM -0400, Guido G?nther wrote:
> On Thu, Jul 24, 2008 at 10:01:32AM +0100, Daniel P. Berrange wrote:
[..snip..]
> > You need to check for UUID clash too.
> Indeed. But before fixing this I wonder what the exact semantics of
> domainCreateLinux are. Is it correct that we don't call
> virDomainSaveConfiguration?
Yes, that is correct.
virDomainCreateLinux() starts a virtual machine with no config file. All
trace of it will disappear when it shuts down - a so called 'transient'
VM.
Alternatively you can define the config first with virDomainDefineXML()
and then start it based on this definition with virDomainCreate(). This
gives you a persistent VM.
Now, while a transient VM is running you can explicitly give it a config
file by called virDomainDefineXML with the same uuid, thus turning it in
to a persistent VM.
The attache patch also checks the uuid.
-- Guido
12:05 a.m.
New subject: [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.
4:30 a.m.
New subject: [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/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
1:49 p.m.
New subject: [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.
2:06 p.m.
New subject: [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.
1:24 a.m.
New subject: [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/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
3:43 a.m.
New subject: [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 :|
6:08 a.m.
New subject: [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
1:26 a.m.
New subject: [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/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5993
days inactive
6001
days old
13 comments
4 participants
participants (4)
-
Charles Duffy -
Daniel P. Berrange -
Daniel Veillard -
Guido Günther