On Thu, Jul 24, 2008 at 08:24:32AM -0400, Daniel Veillard wrote:
> On Tue, Jul 08, 2008 at 05:38:41PM +0100, Daniel P. Berrange wrote:
> > - 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.
Yep, fortunately we have pretty good test coverage of the XML parsing &
formatting for Xen drivers, so it should catch most of the problems :-)
> [...]
> > + 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.
Yep, I added a 'sexpr_node_copy' convenience method to catch OOM conditions
more easily.
> [...]
> > + 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.
Good point.
> > + 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.
That's a nice idea - I'll cook up a patch to do add a VIR_STRDUP
later.
> > 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 ?
The reason the <graphics> and <input> elements are added here
is because of a bug in the test suite - the sexpr2xmldatatest.c
was setting the wrong xendConfigVersion for this particular
test case, so previously it missed the VNC config. Fixing the
test suite meant I had to add in the <graphics> elements.
The <clock> stuf is a new feature - we previously only tracked
the clock setting for HVM guests. Paravirt guests are automatically
synced to UTC, so adding this new element is OK here.
You are correct that we are removing <script> element here, but
I have only done this for the <interface type='bridge'>. It does
not really make sense to customize the script for bringing up an
interface, because then its not really bridging in the sense thta
the libvirt XML describes. THe script parameter is still used for
the generic network config with <interface type='ethernet'>, but
I don't think I have a test case for that.
>
> > 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
Yep, I decided we should be explicit and always add the
<os> block even if we have a bootloader, so people don't
need to special case the paravirt guest config in this
area.
> > - <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 ?
Yes, if there is no explicit 'maxmem' setting, we set it based
on the 'memory' value, and vica-verca. So when outputting the
XML we will always have both elements.
> [...]
> > 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
The parser is a little more fussy here - it expects either the single
string 'all', or a list of models. It's not letting you mix a list of
strings and 'all' at the same time. I will look at fixing this again.
>
> > 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
In this case, it would previously just ignore those bogus values, but
the parser now rejects them explicitly. The real world, if you had a
Xen config with a bogus 'idontexist' you'd get a nice error message
when dumping the XML
"unexpected sound model idontexist"
at lesat you would if I had remembered to call __virRaiseError(), which
I notice I missed in this particular example. So I'll fix that bit.
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list