5:53 a.m.
Cole Robinson <crobinso(a)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);
...