[Libvir] [PATCH] sound support for qemu and xen

[Daniel P. Berrange]

On Mon, Apr 28, 2008 at 01:44:32PM -0400, Cole Robinson wrote:
> Cole Robinson wrote:
> > The patch below adds xml support for the soundhw option to qemu
> > and xen. The new xml element takes the form:
> > 
> 
> Here is the updated patch. Took a bit more work to take into account
> the multiple models, building and parsing the xen soundhw string,
> checking for duplicates, etc.
> 
> The current version uses the format:
> 
> <sound model='m1'/>
> <sound model='m2'/>
> 
> To enable support for models m1 and m2. The code will fail if you
> attempt to define an xml config which specifies a model that isn't
> in the whitelist (currently composed of 'sb16, 'es1370', and 'pcspk').
> Unknown values from a xend sexpr or config file will be silently
> ignored.
> 
> One question: should the value 'all' be recognized from a xen domain
> and translated into a <sound> tag for every item in the whitelist?
> 'all' is an accepted value for a xen domain, since it just passes
> the string to qemu. This isn't in the code but I only thought of it
> now.

Sigh, that's a good question. I think I'd probably turn it into a
list of 3 <sound> tags, so that if they load the XML back in, it'll
get changed to the canonical explicit format.

> +static int qemudParseSoundXML(virConnectPtr conn,
> +                              struct qemud_vm_sound_def *sound,
> +                              xmlNodePtr node) {
> +
> +    xmlChar *model = NULL;
> +    model = xmlGetProp(node, BAD_CAST "model");
> +
> +    if (!model) {
> +        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                         "%s", _("missing sound model"));
> +        goto error;
> +    }
> +    if ((sound->model = qemudSoundModelFromString(conn, (char *) model)) < 0)
> +        goto error;
> +
> +    if (model)
> +        xmlFree(model);
> +    return 0;
> +
> + error:
> +    if (model)
> +        xmlFree(model);
> +    return -1;
> +}

IIRC,  xmlFree already copes with NULL, the 'if()' bits are not required.
You can check with 'make syntax-check' and see if it complains about these
lines.


Aside from this, it looks reasonable - I'd like to see a couple of 
XML configs added to the test cases for xen and QEMU to validate the
parsing and formatting of XML.

Regards,
Dan.
-- 
|: Red Hat, Engineering, Boston   -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 :|