[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] PATCH: 10/14: Convert XenD XML parser to generic API



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 redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]