Cole Robinson <crobinso@xxxxxxxxxx> 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);
...
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list