Re: [alsa-devel] [PATCH v2 3/3] topology: A API calls to directly build topology data from templates

On Mon, 10 Aug 2015 20:13:49 +0200, Liam Girdwood wrote:

+int tplg_add_bytes(snd_tplg_t *tplg, struct snd_tplg_bytes_template *bytes_ctl,

• struct tplg_elem **e)

+{

• struct snd_soc_tplg_bytes_control *be;
• struct tplg_elem *elem;
• int ret;
• tplg_dbg(" Control Bytes: %s\n", bytes_ctl->hdr.name);
• if (bytes_ctl->hdr.type != SND_SOC_TPLG_TYPE_BYTES) {

• SNDERR("error: invalid bytes type %d\n", bytes_ctl->hdr.type);
  

• return -EINVAL;
  

• }
• elem = tplg_elem_new_common(tplg, NULL, bytes_ctl->hdr.name,

• SND_TPLG_TYPE_BYTES);
  

• if (!elem)

• return -ENOMEM;
  

• be = elem->bytes_ext;
• be->size = elem->size;
• ret = init_ctl_hdr(&be->hdr, &bytes_ctl->hdr);
• if (ret < 0) {

• tplg_elem_free(elem);
  

• return ret;
  

• }
• be->max = bytes_ctl->max;
• be->mask = bytes_ctl->mask;
• be->base = bytes_ctl->base;
• be->num_regs = bytes_ctl->num_regs;
• be->ext_ops.put = bytes_ctl->ext_ops.put;
• be->ext_ops.get = bytes_ctl->ext_ops.get;
• if (bytes_ctl->priv != NULL) {

• be = realloc(be,
  

• 	elem->size + bytes_ctl->priv->size);
  

• if (!be) {
  

• 	tplg_elem_free(elem);
  

• 	return -ENOMEM;
  

• }
  

• elem->bytes_ext = be;
  

• elem->size += bytes_ctl->priv->size;
  

• memcpy(be->priv.data, bytes_ctl->priv->data,
  

• 	bytes_ctl->priv->size);
  

• be->priv.size = bytes_ctl->priv->size;
  

• }
• /* check on TLV bytes control */
• if (be->hdr.access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {

• if (be->hdr.access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
  

• 	!= SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) {
  

• 	SNDERR("error: Invalid TLV bytes control access 0x%x\n",
  

• 		be->hdr.access);
  

• 	return -EINVAL;
  

Missing tplg_elem_free()?

+struct tplg_elem* tplg_elem_new_route(snd_tplg_t *tplg) +{

• struct tplg_elem *elem;
• struct snd_soc_tplg_dapm_graph_elem *line;
• elem = tplg_elem_new();
• if (!elem)

• return NULL;
  

• list_add_tail(&elem->list, &tplg->route_list);
• strcpy(elem->id, "line");
• elem->type = SND_TPLG_TYPE_DAPM_GRAPH;
• elem->size = sizeof(*line);
• line = calloc(1, sizeof(*line));
• if (!line)

• return NULL;
  

Missing free of elem in the error path. Also, tplg->route_list still contains the stray elem.

I guess this was a bug in the original code, so you can address it before this patch.

+int tplg_add_widget_object(snd_tplg_t *tplg, snd_tplg_obj_template_t *t) +{

• struct snd_tplg_widget_template *wt = t->widget;
• struct snd_soc_tplg_dapm_widget *w;
• struct tplg_elem *elem;
• int i, ret = 0;
• tplg_dbg("Widget: %s\n", wt->name);
• elem = tplg_elem_new_common(tplg, NULL, wt->name,

• SND_TPLG_TYPE_DAPM_WIDGET);
  

• if (!elem)

• return -ENOMEM;
  

• /* init new widget */
• w = elem->widget;
• w->size = elem->size;
• w->id = wt->id;
• elem_copy_text(w->name, wt->name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
• if (wt->sname)

• elem_copy_text(w->sname, wt->sname,
  

• SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
  

Indentation problem.

• /* add controls to the widget's reference list */
• for (i = 0 ; i < wt->num_ctls; i++) {

• struct snd_tplg_ctl_template *ct = wt->ctl[i];
  

• struct tplg_elem *elem_ctl;
  

• struct snd_tplg_mixer_template *mt;
  

• struct snd_tplg_bytes_template *bt;
  

• struct snd_tplg_enum_template *et;
  

• if (!ct) {
  

• 	tplg_elem_free(elem);
  

• 	return -EINVAL;
  

• }
  

• switch (ct->type) {
  

• case SND_SOC_TPLG_TYPE_MIXER:
  

• 	mt = container_of(ct, struct snd_tplg_mixer_template, hdr);
  

• 	ret = tplg_add_mixer(tplg, mt, &elem_ctl);
  

• 	break;
  

• case SND_SOC_TPLG_TYPE_BYTES:
  

• 	bt = container_of(ct, struct snd_tplg_bytes_template, hdr);
  

• 	ret = tplg_add_bytes(tplg, bt, &elem_ctl);
  

• 	break;
  

• case SND_SOC_TPLG_TYPE_ENUM:
  

• 	et = container_of(ct, struct snd_tplg_enum_template, hdr);
  

• 	ret = tplg_add_enum(tplg, bt, &elem_ctl);
  

et instead of bt?

+int snd_tplg_build(snd_tplg_t *tplg, const char *outfile) +{

• int err;
• /* delete any old output files */
• unlink(outfile);
• tplg->out_fd =

• open(outfile, O_RDWR | O_CREAT, S_IRWXU | S_IRWXG | S_IRWXO);
  

• if (tplg->out_fd < 0) {

• SNDERR("error: failed to open %s err %d\n",
  

• 	outfile, -errno);
  

• return -errno;
  

• }
• err = tplg_build_integ(tplg);
• if (err < 0) {

• SNDERR("error: failed to check topology integrity\n");
  

• goto out;
  

• }
• err = tplg_write_data(tplg);
• if (err < 0) {

• SNDERR("error: failed to write data %d\n", err);
  

• goto out;
  

• }

+out:

• close(tplg->out_fd);

It's a question whether we should unlink automatically at error or not. If we leave the file as is, it should be documented.

diff --git a/src/topology/tplg_local.h b/src/topology/tplg_local.h index 953d822..97ee33e 100644 --- a/src/topology/tplg_local.h +++ b/src/topology/tplg_local.h @@ -34,6 +34,10 @@ #define ALSA_TPLG_DIR ALSA_CONFIG_DIR "/topology" #define ARRAY_SIZE(x) (sizeof(x) / sizeof(x[0]))

+#define container_of(ptr, type, member) ({ \

• const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
  

• (type *)( (char *)__mptr - offsetof(type,member) );})

This is a pretty common macro, so I prefer having it in include/local.h.

thanks,

Takashi