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

29 Apr 2008

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);
...

    

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

Jim Meyering