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

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.

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.

Yep, also fixed on my laptop.

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

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

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