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

[Daniel P. Berrange]

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