Re: [libvirt] PATCH: 3/5: The easy Xen and QEMU implementations

On Wed, May 13, 2009 at 03:38:57PM +0100, Daniel P. Berrange wrote: > This patches provides an implenmentation of the new APIs for Xen which > can convert to/from both XM config format (/etc/xen files), and the > SEXPR format used by XenD. The former is most useful to end users, but > it was easy to add the latter too, so I did. It can be a useful debugging > aid sometimes. For QEMU, it implemnets export of domain XML into the > QEMU argv format. > > --- a/src/qemu_conf.c Wed May 13 13:13:18 2009 +0100 > +++ b/src/qemu_conf.c Wed May 13 13:16:50 2009 +0100 > @@ -1248,21 +1248,18 @@ int qemudBuildCommandLine(virConnectPtr > > case VIR_DOMAIN_NET_TYPE_ETHERNET: > { > - char arg[PATH_MAX]; > - if (net->ifname) { > - if (snprintf(arg, PATH_MAX-1, "tap,ifname=%s,script=%s,vlan=%d", > - net->ifname, > - net->data.ethernet.script, > - vlan) >= (PATH_MAX-1)) > - goto error; > - } else { > - if (snprintf(arg, PATH_MAX-1, "tap,script=%s,vlan=%d", > - net->data.ethernet.script, > - vlan) >= (PATH_MAX-1)) > - goto error; > - } > + virBuffer buf = VIR_BUFFER_INITIALIZER; > > - ADD_ARG_LIT(arg); > + virBufferAddLit(&buf, "tap"); > + if (net->ifname) > + virBufferVSprintf(&buf, ",ifname=%s", net->ifname); > + if (net->data.ethernet.script) > + virBufferVSprintf(&buf, ",script=%s", net->data.ethernet.script); > + virBufferVSprintf(&buf, ",vlan=%d", vlan); > + if (virBufferError(&buf)) > + goto error; > + > + ADD_ARG(virBufferContentAndReset(&buf)); > } > break; Hum, that part looks a bit orthogonal to the patch itself, isn't it ?

[...] > + /* Since we're just exporting args, we can't do bridge/network > + * setups, since libvirt will normally create TAP devices > + * directly. We convert those configs into generic 'ethernet' > + * config and assume the user has suitable 'ifup-qemu' scripts > + */ > + for (i = 0 ; i < def->nnets ; i++) { > + virDomainNetDefPtr net = def->nets[i]; > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { > + VIR_FREE(net->data.network.name); > + net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; > + net->data.ethernet.dev = NULL; > + net->data.ethernet.script = NULL; > + net->data.ethernet.ipaddr = NULL; > + } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { > + net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; > + /* Rely on fact that the 'ethernet' and 'bridge' > + * union structs have members in same place */ Hum, it's a bit fishy... why not play safe, shaving a couple microseconds here is probably not worth it. [...] > +cleanup: > + for (tmp = retargv ; tmp && *tmp ; tmp++) weird, we use a static string just for "" ? > + VIR_FREE(*tmp); > + VIR_FREE(retargv); > + > + for (tmp = retenv ; tmp && *tmp ; tmp++) same

> --- a/src/xen_unified.h Wed May 13 13:13:18 2009 +0100 > +++ b/src/xen_unified.h Wed May 13 13:16:50 2009 +0100 > @@ -46,6 +46,9 @@ extern int xenRegister (void); > > #define MIN_XEN_GUEST_SIZE 64 /* 64 megabytes */ > > +#define XEN_CONFIG_FORMAT_XM "xen-xm" > +#define XEN_CONFIG_FORMAT_SEXPR "xen-sxpr" Actually, shouldn't those be part of the public headers instead I'm afraid I'm missing something here ...

Daniel