On Wed, May 13, 2009 at 05:59:38PM +0200, Daniel Veillard wrote:
> 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 ?
Sort of. The problem was that the original code assumed 'script'
was non-NULL. With the way we call it now, it is not guarenteed
to be non-NULL. Actually it was never really guarenteed non-NULL
ever.
>
> [...]
> > + /* 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
I'm not sure what you mean here ?
What is happening is that qemudBuildCommandLine() gives us
back 2 arrays of strings, char **retargv, and char **retenv.
Normally these are passed straight to execve() as is. In this
case though, we just concatentation all the strings in the 2
arrays together into one big string, and return this to the
caller. So these few lines are just free'ing the 2 arrays we
no longer need.
> > --- 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 ...
You'd pass these kind of values into the 'char *nativeFormat' parameter
but I wasn't really considering them part of the stable ABI/API. The
config formats each driver supports are specific to particular backends
and particular HV versions. So I decided it was better not to expose
them directly. Perhaps we could add them to the capabilities XML
format though, as dynamically exported data, so they're not ABI.
>Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
