[libvirt] PATCH: 10/14: Convert XenD XML parser to generic API
Daniel P. Berrange
berrange at redhat.com
Thu Jul 24 15:49:50 UTC 2008
On Thu, Jul 24, 2008 at 09:32:41AM -0400, Daniel Veillard wrote:
> On Tue, Jul 08, 2008 at 05:39:27PM +0100, Daniel P. Berrange wrote:
>
> > static virCapsPtr
> > -xenHypervisorBuildCapabilities(virConnectPtr conn,
> > - const char *hostmachine,
> > +xenHypervisorBuildCapabilities(const char *hostmachine,
> > int host_pae,
> > char *hvm_type,
> > struct guest_arch *guest_archs,
>
> Sounds fishy obviously some error can occur and not propagating
> the connection how to you carry the error properly to the remote
> connection using the Xen node ? For example virCaps allocation may
> fail no ?
Previously the code for building the virCaps object would be
called every time the virConnectGetCapabilities() API was
invokved, so it'd have a virConnectPtr object.
After this re-factoring the virCaps object is created at the
time the connection is opened, so no virConnectPtr object yet
exists. The reason for this is that the virCaps object is
used in the XML parsers/formatters now so we need it around
all the time.
The errors are still reported - they'll be reported as a
failure to cthe virConnectOpen() call, accessible via the
global virGetLastError() API.
> > @@ -3987,6 +3995,7 @@
> > static int
> > xenDaemonDetachDevice(virDomainPtr domain, const char *xml)
> > {
> > +#if 0
> > char class[8], ref[80];
> >
> > if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) {
> > @@ -3998,6 +4007,8 @@
> > return (-1);
> > return(xend_op(domain->conn, domain->name, "op", "device_destroy",
> > "type", class, "dev", ref, "force", "0", "rm_cfg", "1", NULL));
> > +#endif
> > + return (domain == (void*)xml) ? -1 : -1;
> > }
>
> err, I really don't understand that is that a temporary state which
> will be fixed by a later patch ?
Urgh, I've posted the wrong version of this patch. The one on my machine
implements this correctly.
> [...]
> > + switch (def->type) {
> > + case VIR_DOMAIN_NET_TYPE_BRIDGE:
> > + virBufferVSprintf(buf, "(bridge '%s')", def->data.bridge.brname);
> > + virBufferAddLit(buf, "(script 'vif-bridge')");
>
> hum, see the problem reported in the previous patch. i think we should
> carry the bridge script in the definition structure or loose some functionality
> in the transition.
The issue is that when converting from a SEXPR -> XML format, the Xend
driver does
if (STREQ(script, "vif-bridge"))
type = BRIDGE
else
type = ETHERNET
So, if you pass a custom script for vif-bridge, that check won't match
and thus the XML you get back out is
<interface type='ethernet'>
So, it seems pointless adding <script> element for <interface type='bridge'>
since it'll never change.
> > +static int
> > +xenDaemonFormatSxprSound(virConnectPtr conn,
> > + virDomainSoundDefPtr sound,
> > + virBufferPtr buf)
> > +{
> > + const char *str;
> > + virDomainSoundDefPtr prev = NULL;
> > + if (!sound)
> > + return 0;
> > +
> > + virBufferAddLit(buf, "(soundhw '");
> > + while (sound) {
> > + if (!(str = virDomainSoundModelTypeToString(sound->model))) {
> > + virXendError(conn, VIR_ERR_INTERNAL_ERROR,
> > + _("unexpected sound model %d"), sound->model);
> > + return -1;
> > + }
> > + virBufferVSprintf(buf, "%s%s", prev ? "," : "", str);
> > + prev = sound;
> > + sound = sound->next;
> > + }
>
> based on this I suppose the problem exposed in the previous patch about
> soundhw values, the 2 first values are lost when building the list.
>
> > +#if 0
> > +/**
> > + * virDomainXMLDevID:
> > + * @domain: pointer to domain object
> > + * @xmldesc: string with the XML description
> > + * @class: Xen device class "vbd" or "vif" (OUT)
> > + * @ref: Xen device reference (OUT)
> > + *
> > + * Set class according to XML root, and:
> > + * - if disk, copy in ref the target name from description
> > + * - if network, get MAC address from description, scan XenStore and
> > + * copy in ref the corresponding vif number.
> > + *
> > + * Returns 0 in case of success, -1 in case of failure.
> > + */
>
> hum, missing functionality ?
Yep, also fixed on my laptop.
> > +virCapsPtr testXenCapsInit(void) {
> > + struct utsname utsname;
> > + virCapsPtr caps;
> > + virCapsGuestPtr guest;
> > + static const char *const x86_machines[] = {
> > + "xenfv"
> > + };
> > + static const char *const xen_machines[] = {
> > + "xenpv"
> > + };
> > +
> > + uname (&utsname);
> > + if ((caps = virCapabilitiesNew(utsname.machine,
> > + 0, 0)) == NULL)
> > + return NULL;
>
> I think it's better to pass a NULL here for the connection but still
> keep the normal connection error reporting capabilities in the library
> code.
This is called from virConnectOpen, which is why i removed
the conn parameter, but could add it back.
> > + if ((guest = virCapabilitiesAddGuest(caps, "hvm", "i686", 32,
> > + "/usr/lib/xen/bin/qemu-dm", NULL,
> > + 1, x86_machines)) == NULL)
> > --- a/tests/xml2sexprdata/xml2sexpr-curmem.sexpr Thu Jul 03 12:50:05 2008 +0100
> > +++ b/tests/xml2sexprdata/xml2sexpr-curmem.sexpr Thu Jul 03 13:02:42 2008 +0100
> > @@ -1,1 +1,1 @@
> > -(vm (name 'rhel5')(memory 175)(maxmem 385)(vcpus 1)(uuid '4f77abd2301958e83bab6fbf2118f880')(bootloader '/usr/bin/pygrub')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(device (tap (dev 'xvda:disk')(uname 'tap:aio:/xen/rhel5.img')(mode 'w')))(device (vif (mac '00:16:3e:1d:06:15')(bridge 'xenbr0')(script 'vif-bridge'))))
> > \ No newline at end of file
> > +(vm (name 'rhel5')(memory 175)(maxmem 385)(vcpus 1)(uuid '4f77abd2-3019-58e8-3bab-6fbf2118f880')(bootloader '/usr/bin/pygrub')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(device (tap (dev 'xvda:disk')(uname 'tap:aio:/xen/rhel5.img')(mode 'w')))(device (vif (mac '00:16:3e:1d:06:15')(bridge 'xenbr0')(script 'vif-bridge'))))
>
> Hum, we changed the UUID output format ! Isn't there a danger here ? I'm
> not sure all versions of Xend will be smart ...
The xm_internal.c driver already used the format including '-', but
I could change this back if desired.
> [...]
> > diff -r ae83be3e7918 tests/xml2sexprdata/xml2sexpr-fv-localtime.xml
> > --- a/tests/xml2sexprdata/xml2sexpr-fv-localtime.xml Thu Jul 03 12:50:05 2008 +0100
> > +++ b/tests/xml2sexprdata/xml2sexpr-fv-localtime.xml Thu Jul 03 13:02:42 2008 +0100
> > @@ -29,7 +29,7 @@
> > </disk>
> > <disk type='file'>
> > <source file='/root/foo.img'/>
> > - <target dev='ioemu:hda'/>
> > + <target dev='hda'/>
> > </disk>
> > <graphics type='vnc' port='5917' keymap='ja'/>
> > </devices>
>
> hum, why changing the inputs ? we can't parse it ?
Actaully we can parse & ignore this prefix. I can get rid of
this tweak to the XML - its left-over from an intermediate
version of my patch.
> > diff -r ae83be3e7918 tests/xml2sexprdata/xml2sexpr-no-source-cdrom.xml
> > --- a/tests/xml2sexprdata/xml2sexpr-no-source-cdrom.xml Thu Jul 03 12:50:05 2008 +0100
> > +++ b/tests/xml2sexprdata/xml2sexpr-no-source-cdrom.xml Thu Jul 03 13:02:42 2008 +0100
> > @@ -25,7 +25,7 @@
> > <disk type='block' device='disk'>
> > <driver name='phy'/>
> > <source dev='/dev/sda8'/>
> > - <target dev='hda:disk'/>
> > + <target dev='hda'/>
> > </disk>
> > <disk device='cdrom'>
> > <target dev='hdc'/>
>
> agreed the input looks a bit stange there :-)
Yeah, the corresponding SXPR was wrong too - simple test case
bug.
> > +++ b/tests/xml2sexprdata/xml2sexpr-pv-vfb-new.sexpr Thu Jul 03 13:02:42 2008 +0100
> > @@ -1,1 +1,1 @@
> > -(vm (name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d2171f48fb2e068e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (linux (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_64/os ')))(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w')))(device (vkbd))(device (vfb (type vnc)(vncdisplay 6)(vnclisten 127.0.0.1)(vncpasswd 123456)(keymap ja))))
> > \ No newline at end of file
> > +(vm (name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d21-71f4-8fb2-e068-e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (linux (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_64/os ')(device_model '/usr/lib/xen/bin/qemu-dm')))(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w')))(device (vkbd))(device (vfb (type vnc)(vncunused 0)(vncdisplay 6)(vnclisten '127.0.0.1')(vncpasswd '123456')(keymap 'ja'))))
>
>
> Hum we lost more informations here:
> (device_model '/usr/lib/xen/bin/qemu-dm')
We *added* the 'device_model' actually :-)
> > diff -r ae83be3e7918 tests/xml2sexprdata/xml2sexpr-pv-vfb-orig.sexpr
> > --- a/tests/xml2sexprdata/xml2sexpr-pv-vfb-orig.sexpr Thu Jul 03 12:50:05 2008 +0100
> > +++ b/tests/xml2sexprdata/xml2sexpr-pv-vfb-orig.sexpr Thu Jul 03 13:02:42 2008 +0100
> > @@ -1,1 +1,1 @@
> > -(vm (name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d2171f48fb2e068e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (linux (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_64/os ')(vnc 1)(vncdisplay 6)(vnclisten 127.0.0.1)(vncpasswd 123456)(keymap ja)))(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w'))))
> > \ No newline at end of file
> > +(vm (name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d21-71f4-8fb2-e068-e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (linux (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_64/os ')(vnc 1)(vncunused 0)(vncdisplay 6)(vnclisten '127.0.0.1')(vncpasswd '123456')(keymap 'ja')))(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w'))))
>
> We add (vncunused 0) I must admit i don't see what it could mean in that
> context the input is
> <graphics type='vnc' port='5906' listen="127.0.0.1" passwd="123456" keymap="ja"/>
It is a safety net - we have an explicit port, given via (vncdisplay 6),
but adding the (vncunused 0) is just a safety net incase XenD changes
its default handling - one version of xen-unstable did this before,
but we caught it before release.
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 :|
More information about the libvir-list
mailing list