[libvirt] PATCH: 10/14: Convert XenD XML parser to generic API
Daniel Veillard
veillard at redhat.com
Thu Jul 24 13:32:41 UTC 2008
On Tue, Jul 08, 2008 at 05:39:27PM +0100, Daniel P. Berrange wrote:
> This replaces the code which converts from the XML into SEXPR
> with code which converts from the virDomainDefPtr object to a
> SEXPR. We then simply use virDomainDefParseString() to generate
> the initial virDomainDefPtr. This makes the SEXPR generating
> code much easier to follow.
okay
> As part of this cleaned the code all moved out of xml.c and
> into xend_internal.c so its in the same place as its inverse
> SEXPR -> XML code. I'm half-inclined to actually move all
> the SEXPR related code into a xen_conf.{c,h} file independant
> of the main driver routine. This would reflect the split we
> for QEMU, LXC & OpenVZ drivers.
might make sense, but not urgent
> 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 ?
> @@ -2188,7 +2187,7 @@
>
>
> if (sys_interface_version >= 4) {
> - if (xenDaemonNodeGetTopology(conn, caps) != 0) {
> + if (xenDaemonNodeGetTopology(NULL, caps) != 0) {
Hum, why ?
> -char *
> -xenHypervisorMakeCapabilitiesXML(virConnectPtr conn,
> - const char *hostmachine,
> - FILE *cpuinfo, FILE *capabilities)
> +virCapsPtr
> +xenHypervisorMakeCapabilitiesInternal(const char *hostmachine,
> + FILE *cpuinfo, FILE *capabilities)
I don't understand really
> +/**
> + * xenHypervisorMakeCapabilities:
> + *
> + * Return the capabilities of this hypervisor.
> + */
> +virCapsPtr
> +xenHypervisorMakeCapabilities(void)
> +{
> + virCapsPtr caps;
> + FILE *cpuinfo, *capabilities;
> + struct utsname utsname;
> +
> + /* Really, this never fails - look at the man-page. */
> + uname (&utsname);
> +
> + cpuinfo = fopen ("/proc/cpuinfo", "r");
> + if (cpuinfo == NULL) {
> + if (errno != ENOENT) {
> + virXenPerror (NULL, "/proc/cpuinfo");
> + return NULL;
> + }
> + }
> +
> + capabilities = fopen ("/sys/hypervisor/properties/capabilities", "r");
> + if (capabilities == NULL) {
> + if (errno != ENOENT) {
> + fclose(cpuinfo);
> + virXenPerror (NULL, "/sys/hypervisor/properties/capabilities");
> + return NULL;
> + }
> + }
> +
> + caps = xenHypervisorMakeCapabilitiesInternal(utsname.machine, cpuinfo, capabilities);
> +
> + if (cpuinfo)
> + fclose(cpuinfo);
> + if (capabilities)
> + fclose(capabilities);
> +
> + return caps;
> +}
I really think the connection should be passed down here.
[...]
> @@ -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 ?
[...]
> + 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.
> +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 ?
[...]
> diff -r ae83be3e7918 tests/testutilsxen.c
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/testutilsxen.c Thu Jul 03 13:02:42 2008 +0100
> @@ -0,0 +1,53 @@
> +#include <config.h>
> +
> +#include <sys/utsname.h>
> +#include <stdlib.h>
> +
> +#include "testutilsxen.h"
> +
> +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.
> + 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 ...
[...]
> 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 ?
> 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 :-)
> +++ 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')
> 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"/>
Basically same as the previous one, i guess it's globally okay, there is a
few issues, one stylistic and the others on the outputs but which can be
adressed as separate patches on top.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard at redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
More information about the libvir-list
mailing list