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

[Daniel Veillard]

On Thu, Jul 24, 2008 at 04:49:50PM +0100, Daniel P. Berrange wrote:
> 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.

  Ah, okay, I remember now this been discussed, but i had forgotten !

> Urgh, I've posted the wrong version of this patch. The one on my machine
> implements this correctly.

  haha, :-)

> > [...]
> > > +    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.

  okay, that removes my concern, there is no loss of functionlity possible.

> Yep, also fixed on my laptop.

  Let's push the final version to CVS, really !

> >    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.

  nahh as long as it doesn't break, I don't care

> > > -      <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.

  okay

> >    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.

  Okay +1

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/