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