[libvirt] [PATCH] save state as string
Hi, current domstatus code saves the domain as number, attached patch safes it as string which looks a bit nicer. Cheers, -- Guido
On Mon, Dec 29, 2008 at 03:44:54PM +0100, Guido Günther wrote:
Hi, current domstatus code saves the domain as number, attached patch safes it as string which looks a bit nicer. [...] @@ -1414,7 +1416,9 @@ qemudDomainStatusFormat(virConnectPtr conn, char *config_xml = NULL, *xml = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER;
- virBufferVSprintf(&buf, "<domstatus state='%d' pid='%d'>\n", vm->state, vm->pid); + virBufferVSprintf(&buf, "<domstatus state='%s' pid='%d'>\n", + virDomainStateTypeToString(vm->state), + vm->pid); virBufferEscapeString(&buf, " <monitor path='%s'/>\n", vm->monitorpath);
if (!(config_xml = virDomainDefFormat(conn,
Hum, I have 2 objections to this patch: - seems we keep a string value internally in the structure, I don't see the benefit, or maybe I misunderstood. - second more important is that we change the XML interface, that we can't do, maybe we can extend it to provide a stateinfo="%s" along the state='%d', but we can't just break code which may rely on the existing format. maybe I missed something though, 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/
On Tue, Jan 06, 2009 at 03:27:52PM +0100, Daniel Veillard wrote:
On Mon, Dec 29, 2008 at 03:44:54PM +0100, Guido Günther wrote:
Hi, current domstatus code saves the domain as number, attached patch safes it as string which looks a bit nicer. [...] @@ -1414,7 +1416,9 @@ qemudDomainStatusFormat(virConnectPtr conn, char *config_xml = NULL, *xml = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER;
- virBufferVSprintf(&buf, "<domstatus state='%d' pid='%d'>\n", vm->state, vm->pid); + virBufferVSprintf(&buf, "<domstatus state='%s' pid='%d'>\n", + virDomainStateTypeToString(vm->state), + vm->pid); virBufferEscapeString(&buf, " <monitor path='%s'/>\n", vm->monitorpath);
if (!(config_xml = virDomainDefFormat(conn,
Hum, I have 2 objections to this patch: - seems we keep a string value internally in the structure, I don't see the benefit, or maybe I misunderstood. Currently we have state='1' for running, with this patch this reads state='running' which is better readable.
- second more important is that we change the XML interface, that we can't do, maybe we can extend it to provide a stateinfo="%s" along the state='%d', but we can't just break code which may rely on the existing format.
The file we write is /var/run/libvirt/qemu/*.xml where we keep the internal state of a running vm. We can change this at at any time. Cheers, -- Guido
On Tue, Jan 06, 2009 at 04:38:50PM +0100, Guido G?nther wrote:
On Tue, Jan 06, 2009 at 03:27:52PM +0100, Daniel Veillard wrote:
On Mon, Dec 29, 2008 at 03:44:54PM +0100, Guido Günther wrote:
Hi, current domstatus code saves the domain as number, attached patch safes it as string which looks a bit nicer. [...] @@ -1414,7 +1416,9 @@ qemudDomainStatusFormat(virConnectPtr conn, char *config_xml = NULL, *xml = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER;
- virBufferVSprintf(&buf, "<domstatus state='%d' pid='%d'>\n", vm->state, vm->pid); + virBufferVSprintf(&buf, "<domstatus state='%s' pid='%d'>\n", + virDomainStateTypeToString(vm->state), + vm->pid); virBufferEscapeString(&buf, " <monitor path='%s'/>\n", vm->monitorpath);
if (!(config_xml = virDomainDefFormat(conn,
Hum, I have 2 objections to this patch: - seems we keep a string value internally in the structure, I don't see the benefit, or maybe I misunderstood. Currently we have state='1' for running, with this patch this reads state='running' which is better readable.
- second more important is that we change the XML interface, that we can't do, maybe we can extend it to provide a stateinfo="%s" along the state='%d', but we can't just break code which may rely on the existing format.
The file we write is /var/run/libvirt/qemu/*.xml where we keep the internal state of a running vm. We can change this at at any time.
Yes & no, but mostly no. These persistent state files will need to be stable across releases of libvirt, to allow sensible live upgrade path for hosts. eg, you're running libvirtd with several VMs, upgrade to latest RPM which does a 'service libvirtd restart' in the %post install section. The new libvirtd needs to cope with the state file created by the old libvirtd. In this particular case though, its not problem changing state=1 to state=running, since we've not released this code yet and the named attribute is nicer to read. 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 :|
The file we write is /var/run/libvirt/qemu/*.xml where we keep the internal state of a running vm. We can change this at at any time.
Yes & no, but mostly no.
These persistent state files will need to be stable across releases of libvirt, to allow sensible live upgrade path for hosts. eg, you're running libvirtd with several VMs, upgrade to latest RPM which does a 'service libvirtd restart' in the %post install section. The new libvirtd needs to cope with the state file created by the old libvirtd. Maybe I was a bit unclear, what I meant to say is: This is internal to
On Tue, Jan 06, 2009 at 05:51:27PM +0000, Daniel P. Berrange wrote: [..snip..] libvirt so if we change the XML format it's only *us* who need to worry about what changed. E.g. in this case (if there'd been any releases with this code) we'd have to keep number and string parsing for state=....
In this particular case though, its not problem changing state=1 to state=running, since we've not released this code yet and the named attribute is nicer to read. So o.k. to apply this change? -- Guido
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Guido Günther