On Tue, Jul 08, 2008 at 05:38:41PM +0100, Daniel P. Berrange wrote:
> This replaces the code which converts from the SEXPR into XML
> with code which converts from the SEXPR to virDomainDefPtr
> object. We then simply call virDomainDefFormat() to generate
> the XML. The generated XML is thus guarenteed consistent with
> the QEMU driver & others using domain_conf.c routines.
> Nearly all of the XML files had to be updated because the generic
> XML formatter outputs various elements in an alternate order than
> the Xen driver did.
okay
> +int sexpr_node_copy(const struct sexpr *sexpr, const char *node, char **dst)
> +{
> + const char *val = sexpr_node(sexpr, node);
> +
> + if (val) {
> + *dst = strdup(val);
Hum we really ought to use the new macros here and raise the error, no ?
> + if (!(*dst))
> + return -1;
> + } else {
> + *dst = NULL;
> + }
> + return 0;
> +}
> +
> +
[...]
> - const char *boot = sexpr_node(node, "domain/image/hvm/boot");
> - if ((boot != NULL) && (boot[0] != 0)) {
> - while (*boot) {
> - if (*boot == 'a')
> - /* XXX no way to deal with boot from 2nd floppy */
> - virBufferAddLit(buf, " <boot dev='fd'/>\n");
> - else if (*boot == 'c')
> - /*
> - * Don't know what to put here. Say the vm has been given 3
> - * disks - hda, hdb, hdc. How does one identify the boot disk?
> - * We're going to assume that first disk is the boot disk since
> - * this is most common practice
> - */
> - virBufferAddLit(buf, " <boot dev='hd'/>\n");
> - else if (*boot == 'd')
> - virBufferAddLit(buf, " <boot dev='cdrom'/>\n");
> - else if (*boot == 'n')
> - virBufferAddLit(buf, " <boot dev='network'/>\n");
> - boot++;
> - }
> - }
Hum, the logic in virDomainDefFormat() seems rather different, I hope
this won't lead to too many regressions.
[...]
> + if (sexpr_node_copy(node, "domain/image/hvm/kernel", &def->os.kernel) < 0)
> + goto no_memory;
so you raise the memory issue here, i guess that's fine too.
> + if (!def->os.kernel &&
> + hvm) {
> + const char *boot = sexpr_node(node, "domain/image/hvm/boot");
> + if ((boot != NULL) && (boot[0] != 0)) {
> + while (*boot &&
> + def->os.nBootDevs < VIR_DOMAIN_BOOT_LAST) {
> + if (*boot == 'a')
> + def->os.bootDevs[def->os.nBootDevs++] = VIR_DOMAIN_BOOT_FLOPPY;
> + else if (*boot == 'c')
> + def->os.bootDevs[def->os.nBootDevs++] = VIR_DOMAIN_BOOT_DISK;
> + else if (*boot == 'd')
> + def->os.bootDevs[def->os.nBootDevs++] = VIR_DOMAIN_BOOT_CDROM;
> + else if (*boot == 'n')
> + def->os.bootDevs[def->os.nBootDevs++] = VIR_DOMAIN_BOOT_NET;
> + boot++;
> + }
> + }
> + }
okay, the same logic is actually carried that way, fine.
[...]
> + if (tmp) {
> + unsigned int mac[6];
> + sscanf(tmp, "%02x:%02x:%02x:%02x:%02x:%02x",
> + (unsigned int*)&mac[0],
> + (unsigned int*)&mac[1],
> + (unsigned int*)&mac[2],
> + (unsigned int*)&mac[3],
> + (unsigned int*)&mac[4],
> + (unsigned int*)&mac[5]);
checking that the call returned 6 could be a good idea.
> + net->mac[0] = mac[0];
> + net->mac[1] = mac[1];
> + net->mac[2] = mac[2];
> + net->mac[3] = mac[3];
> + net->mac[4] = mac[4];
> + net->mac[5] = mac[5];
> + }
> +
[...]
> + if (tmp &&
> + !(net->data.ethernet.ipaddr = strdup(tmp)))
> + goto no_memory;
hum, VIR_STRDUP macro and a hook into VIR_ALLOC would be good,
at least it would allow to regression tests on out of memory situations
in that part which is checked as part of make check. But that can be done
separately of course.
> diff -r 3dea6bbe639b tests/sexpr2xmldata/sexpr2xml-curmem.xml
> --- a/tests/sexpr2xmldata/sexpr2xml-curmem.xml Thu Jul 03 11:42:42 2008 +0100
> +++ b/tests/sexpr2xmldata/sexpr2xml-curmem.xml Thu Jul 03 12:50:05 2008 +0100
> @@ -1,6 +1,9 @@
> <domain type='xen' id='5'>
> <name>rhel5</name>
> <uuid>4f77abd2-3019-58e8-3bab-6fbf2118f880</uuid>
> + <memory>394240</memory>
> + <currentMemory>179200</currentMemory>
> + <vcpu>1</vcpu>
> <bootloader>/usr/bin/pygrub</bootloader>
> <os>
> <type>linux</type>
> @@ -8,9 +11,7 @@
> <initrd>/var/lib/xen/initrd.gULTf1</initrd>
> <cmdline>ro root=/dev/VolGroup00/LogVol00 rhgb quiet</cmdline>
> </os>
> - <memory>394240</memory>
> - <currentMemory>179200</currentMemory>
> - <vcpu>1</vcpu>
> + <clock offset='utc'/>
> <on_poweroff>destroy</on_poweroff>
> <on_reboot>restart</on_reboot>
> <on_crash>restart</on_crash>
> @@ -21,15 +22,14 @@
> <target dev='xvda' bus='xen'/>
> </disk>
> <interface type='bridge'>
> + <mac address='00:16:3e:1d:06:15'/>
> <source bridge='xenbr0'/>
> <target dev='vif5.0'/>
> - <mac address='00:16:3e:1d:06:15'/>
> - <script path='vif-bridge'/>
> </interface>
> - <input type='mouse' bus='xen'/>
> - <graphics type='vnc' port='-1'/>
> <console type='pty'>
> <target port='0'/>
> </console>
> + <input type='mouse' bus='xen'/>
> + <graphics type='vnc' port='-1' autoport='yes'/>
> </devices>
> </domain>
Hum, I guess the automatic addition of <clock offset='utc'/> ,
<input type='mouse' bus='xen'/> and <graphics type='vnc' port='-1' autoport='yes'/>
aren't a problem, i.e. not changing the default behaviour, but it seems
we are loosing the <script path='vif-bridge'/> information on the way out,
and that looks significant, right ?
> diff -r 3dea6bbe639b tests/sexpr2xmldata/sexpr2xml-disk-block-shareable.xml
> --- a/tests/sexpr2xmldata/sexpr2xml-disk-block-shareable.xml Thu Jul 03 11:42:42 2008 +0100
> +++ b/tests/sexpr2xmldata/sexpr2xml-disk-block-shareable.xml Thu Jul 03 12:50:05 2008 +0100
> @@ -1,10 +1,14 @@
> <domain type='xen' id='6'>
> <name>pvtest</name>
> <uuid>49a0c6ff-c066-5392-6498-3632d093c2e7</uuid>
> - <bootloader>/usr/bin/pygrub</bootloader>
> <memory>524288</memory>
> <currentMemory>393216</currentMemory>
> <vcpu>1</vcpu>
> + <bootloader>/usr/bin/pygrub</bootloader>
> + <os>
> + <type>linux</type>
> + </os>
pygrub probanly means a linux boot right, but
> - <script path='vif-bridge'/>
lost script again.
[...]
> +++ b/tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml Thu Jul 03 12:50:05 2008 +0100
> @@ -1,20 +1,21 @@
> <domain type='xen' id='3'>
> <name>fvtest</name>
> <uuid>b5d70dd2-75cd-aca5-1776-9660b059d8bc</uuid>
> + <memory>409600</memory>
> + <currentMemory>409600</currentMemory>
> + <vcpu>1</vcpu>
> <os>
> <type>hvm</type>
> <loader>/usr/lib/xen/boot/hvmloader</loader>
> <boot dev='hd'/>
> </os>
> - <memory>409600</memory>
> - <vcpu>1</vcpu>
> - <on_poweroff>destroy</on_poweroff>
> - <on_reboot>restart</on_reboot>
> - <on_crash>restart</on_crash>
> <features>
> <acpi/>
> </features>
> <clock offset='localtime'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>restart</on_crash>
> <devices>
> <emulator>/usr/lib64/xen/bin/qemu-dm</emulator>
> <disk type='file' device='disk'>
We create currentMemory from memory value or I'm mistaken ?
[...]
> diff -r 3dea6bbe639b tests/sexpr2xmldata/sexpr2xml-fv-sound-all.sexpr
> --- a/tests/sexpr2xmldata/sexpr2xml-fv-sound-all.sexpr Thu Jul 03 11:42:42 2008 +0100
> +++ b/tests/sexpr2xmldata/sexpr2xml-fv-sound-all.sexpr Thu Jul 03 12:50:05 2008 +0100
> @@ -1,1 +1,1 @@
> -(domain (domid 3)(name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid 'b5d70dd275cdaca517769660b059d8bc')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(image (hvm (kernel '/usr/lib/xen/boot/hvmloader')(device_model '/usr/lib64/xen/bin/qemu-dm')(boot c)(cdrom '/root/boot.iso')(acpi 1)(vnc 1)(keymap ja)(soundhw 'idontexit,es1370,all')))(device (vbd (dev 'ioemu:hda')(uname 'file:/root/foo.img')(mode 'w')))(device (vif (mac '00:16:3e:1b:b1:47')(bridge 'xenbr0')(script 'vif-bridge')(type ioemu))))
> +(domain (domid 3)(name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid 'b5d70dd275cdaca517769660b059d8bc')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(image (hvm (kernel '/usr/lib/xen/boot/hvmloader')(device_model '/usr/lib64/xen/bin/qemu-dm')(boot c)(cdrom '/root/boot.iso')(acpi 1)(vnc 1)(keymap ja)(soundhw 'all')))(device (vbd (dev 'ioemu:hda')(uname 'file:/root/foo.img')(mode 'w')))(device (vif (mac '00:16:3e:1b:b1:47')(bridge 'xenbr0')(script 'vif-bridge')(type ioemu))))
hum hard to decript, but here it seems we lost the value of the sound
emulation, going from 'idontexit,es1370,all' to 'all' this looks fishy
> diff -r 3dea6bbe639b tests/sexpr2xmldata/sexpr2xml-fv-sound.sexpr
> --- a/tests/sexpr2xmldata/sexpr2xml-fv-sound.sexpr Thu Jul 03 11:42:42 2008 +0100
> +++ b/tests/sexpr2xmldata/sexpr2xml-fv-sound.sexpr Thu Jul 03 12:50:05 2008 +0100
> @@ -1,1 +1,1 @@
> -(domain (domid 3)(name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid 'b5d70dd275cdaca517769660b059d8bc')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(image (hvm (kernel '/usr/lib/xen/boot/hvmloader')(device_model '/usr/lib64/xen/bin/qemu-dm')(boot c)(cdrom '/root/boot.iso')(acpi 1)(vnc 1)(keymap ja)(soundhw 'sb16,es1370,idontexist,es1370more')))(device (vbd (dev 'ioemu:hda')(uname 'file:/root/foo.img')(mode 'w')))(device (vif (mac '00:16:3e:1b:b1:47')(bridge 'xenbr0')(script 'vif-bridge')(type ioemu))))
> +(domain (domid 3)(name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid 'b5d70dd275cdaca517769660b059d8bc')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(image (hvm (kernel '/usr/lib/xen/boot/hvmloader')(device_model '/usr/lib64/xen/bin/qemu-dm')(boot c)(cdrom '/root/boot.iso')(acpi 1)(vnc 1)(keymap ja)(soundhw 'sb16,es1370')))(device (vbd (dev 'ioemu:hda')(uname 'file:/root/foo.img')(mode 'w')))(device (vif (mac '00:16:3e:1b:b1:47')(bridge 'xenbr0')(script 'vif-bridge')(type ioemu))))
Same thing soundhw went from 'sb16,es1370,idontexist,es1370more' to
'sb16,es1370', it's a bit weird
Okidoc, so it looks basically okay to me except the loss of the
network bridging script and the strange thing happening to sound hardware
descriptions. Maybe the simplest is to commit and fix the 2 issues
after with a second patch because rereviewing the whole is while just
a few lines need fixing sounds inefficient :-)
Excellent thing that we have all those regression tests on the SXP<->XML
data !!!
thanks !
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list