[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



Cole Robinson <crobinso redhat com> 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:
...
> Again, this needs to be rediff'd around recent commits (virBuffer
> changes, probably others), which I will do next round after any
> feedback.

Hi Cole,

Yes, parsing in C is no fun...

> diff --git a/src/qemu_conf.c b/src/qemu_conf.c
> index d9b82b2..1b68806 100644
> --- a/src/qemu_conf.c
> +++ b/src/qemu_conf.c
> @@ -1011,6 +1011,64 @@ static int qemudParseInputXML(virConnectPtr conn,
>      return -1;
>  }
>
> +/* Sound device helper functions */
> +static int qemudSoundModelFromString(virConnectPtr conn,
> +                                      const char *model) {

Please don't require a "conn" argument in functions like this.
Instead, let the caller handle any diagnostic.
For an example where this would be useful, see below.

> +    if (STREQ(model, "sb16")) {
> +        return QEMU_SOUND_SB16;
> +    } else if (STREQ(model, "es1370")) {
> +        return QEMU_SOUND_ES1370;
> +    } else if (STREQ(model, "pcspk")) {
> +        return QEMU_SOUND_PCSPK;
> +    }
> +
> +    qemudReportError(conn, NULL, NULL, VIR_ERR_INVALID_ARG,
> +                     _("invalid sound model '%s'"), model);
> +    return -1;
> +}
> +
> +static const char *qemudSoundModelToString(virConnectPtr conn,
> +                                            const int model) {

Likewise.

> +    if (model == QEMU_SOUND_SB16) {
> +        return "sb16";
> +    } else if (model == QEMU_SOUND_ES1370) {
> +        return "es1370";
> +    } else if (model == QEMU_SOUND_PCSPK) {
> +        return "pcspk";
> +    }
> +
> +    qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                     _("invalid sound model '%d'"), model);
> +    return NULL;
> +}
> +
> +
> +static int qemudParseSoundXML(virConnectPtr conn,
> +                              struct qemud_vm_sound_def *sound,
> +                              xmlNodePtr node) {

Can this be "const"?  It looks like it should be.

> +                              const xmlNodePtr node) {

Maybe omit "conn" here too.  Maybe.
Callers could say "missing or invalid sound model", but that's
slightly degraded from the current "invalid sound model '%s'".

> +    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;

It's better to record "int err = -1;" initially, and set
err = 0 if all goes well.  Then you can have a single point
of return and avoid the duplicate xmlFree, e.g.,

    err = 0;

 error:
    xmlFree(model);
    return err;

...
> +    /* Add sound hardware */
> +    if (sound) {
> +        int size = 100;
> +        char *modstr = calloc(1, size+1);
> +        if (!modstr)
> +            goto no_memory;
> +        if (!((*argv)[++n] = strdup("-soundhw")))
> +            goto no_memory;
> +
> +        while(sound && size > 0) {
> +            const char *model = qemudSoundModelToString(conn, sound->model);
> +            strncat(modstr, model, size);

qemudSoundModelToString can return NULL, so you won't want to
use strncat or strlen on that.

...
> +char *sound_string_to_xml(const char *sound) {
> +
> +    char *comma, *model, *dupe;
> +    virBuffer buf;
> +    int collision, modelsize;

Most of the above (but e.g., not "buf") declarations
can be moved "down" into the scope where they're used.

> +    if (!(buf.content = calloc(1, 1024)))
> +        return NULL;
> +    buf.size = 1024;
> +    buf.use = 0;
> +
> +    while (sound) {
> +
> +        collision = 0;
> +        model = NULL;
> +        modelsize = strlen(sound);
> +        if ((comma = strchr(sound, ','))) {
> +            modelsize -= strlen(comma);
> +        }

slightly more efficient, and clearer to me:

    comma = strchr(sound, ',');
    modelsize = comma ? comma - sound : strlen (sound);

> +
> +        // Parse out first element up to comma
> +        if (!strncmp(sound, "sb16", modelsize)) {
> +            model = strdup("sb16");
> +        } else if (!strncmp(sound, "es1370", modelsize)) {
> +            model = strdup("es1370");
> +        } else if (!strncmp(sound, "pcspk", modelsize)) {
> +            model = strdup("pcspk");
> +        }

You can avoid enumerating the types here.  E.g.,
  model = strndup(sound, modelsize);
  if (!model)
      continue;

  if ((m = qemudSoundModelFromString(model)) < 0) {
      free(model);
      continue;
  }

The duplicate detection below misses when a real match is obscured by a
partial one, e.g., a string like "es1370.,es1370" where strstr matches,
but a subsequent word-boundary test fails.
How about a cheaper test, e.g., (with this declaration and initialization
above: char seenSoundModel[20]; memset(seenSoundModel,0,sizeof seenSoundModel);

  assert (m < sizeof seenSoundModel);
  if (seenSoundModel[m])
      collision = 1;
  seenSoundModel[m] = 1;

> +        // Check that model is not already in remaining soundstr
> +        if (comma && model && (dupe = strstr(comma, model))) {
> +            if (( (dupe == sound) ||                //(Start of line |
> +                  (*(dupe - 1) == ',') ) &&         // Preceded by comma) &
> +                ( (dupe[strlen(model)] == ',') ||   //(Ends with comma |
> +                  (dupe[strlen(model)] == '\0') ))  // Ends whole string)
> +                collision = 1;
> +        }
...
>      /* HVM guests, or old PV guests use this config format */
> @@ -1040,6 +1052,10 @@ char *xenXMDomainFormatXML(virConnectPtr conn, virConfPtr conf) {
>      buf->content = NULL;
>      virBufferFree(buf);
>      return (xml);
> +
> +  error:
> +    virBufferFree(buf);
> +    return (NULL);

You can avoid this duplication, too.
As suggested above except here with "char *ret = NULL;", etc.
and "return xml;"

>  }
...
> +char * virBuildSoundStringFromXML(virConnectPtr conn,
> +                                  xmlXPathContextPtr ctxt) {
> +
> +    int nb_nodes, size = 256;
> +    char *dupe, *sound;
> +    xmlNodePtr *nodes = NULL;
> +
> +    if (!(sound = calloc(1, size+1))) {
> +        virXMLError(conn, VIR_ERR_NO_MEMORY,
> +                    _("failed to allocate sound string"), 0);
> +        return NULL;
> +    }
> +
> +    nb_nodes = virXPathNodeSet("/domain/devices/sound", ctxt, &nodes);
> +    if (nb_nodes > 0) {
> +        int i;
> +        for (i = 0; i < nb_nodes && size > 0; i++) {
> +            char *model = NULL;
> +            int collision = 0;
> +
> +            model = (char *) xmlGetProp(nodes[i], (xmlChar *) "model");
> +            if (!model) {
> +                virXMLError(conn, VIR_ERR_XML_ERROR,
> +                            _("no model for sound device"), 0);
> +                goto error;
> +            }
> +
> +            if (!(STREQ(model, "pcspk")||
> +                  STREQ(model, "sb16") ||
> +                  STREQ(model, "es1370"))) {

Don't repeat this list of literal strings.
Imagine having to add a new model type.
You'll want that change to require modification of a minimum number of
places in the code (probably just 2: convert from string to int and int
to string).

Instead, just test qemudSoundModelFromString(model) < 0

> +                virXMLError(conn, VIR_ERR_XML_ERROR,
> +                            _("unknown sound model type"), 0);
> +                free(model);
> +                goto error;
> +            }
> +
> +            // Check for duplicates in currently built string
> +            if (*sound && (dupe = strstr(sound, model))) {

This code looks way too similar to the dup-detecting code above.
Once the above is in a shape you like, consider factoring it
out into a function and reusing that function here.

> +                if (( (dupe == sound) ||                //(Start of line |
> +                      (*(dupe - 1) == ',') ) &&         // Preceded by comma) &
> +                    ( (dupe[strlen(model)] == ',') ||   //(Ends with comma |
> +                      (dupe[strlen(model)] == '\0') ))  // Ends whole string)
> +                    collision = 1;
> +            }
> +
> +            // If no collision, add to string
> +            if (!collision) {
> +                if (*sound && (size >= (strlen(model) + 1))) {
> +                    strncat(sound, ",", size--);
> +                } else if (*sound || size < strlen(model)) {
> +                    free(model);
> +                    continue;
> +                }
> +                strncat(sound, model, size);
> +                size -= strlen(model);
...


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]