[alsa-devel] [PATCH v4] topology: Return -EINVAL at once on failure to find a reference
From: Mengdong Lin mengdong.lin@linux.intel.com
In building phase, return error -EINVAL at once if an element's references (e.g. TLV, text, control or private data) cannot be found.
Move this error check right after looking up a reference, to make the code more clear. And checking the return value of tplg_copy_data() is enough, no need to check ref->elem for data references.
Signed-off-by: Mengdong Lin mengdong.lin@linux.intel.com
diff --git a/src/topology/ctl.c b/src/topology/ctl.c index 54f8e44..d910230 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -135,24 +135,23 @@ static int tplg_build_mixer_control(snd_tplg_t *tplg, if (ref->type == SND_TPLG_TYPE_TLV) { ref->elem = tplg_elem_lookup(&tplg->tlv_list, ref->id, SND_TPLG_TYPE_TLV); - if (ref->elem) - err = copy_tlv(elem, ref->elem); + if (!ref->elem) + goto ref_missing;
+ copy_tlv(elem, ref->elem); } else if (ref->type == SND_TPLG_TYPE_DATA) { err = tplg_copy_data(tplg, elem, ref); if (err < 0) return err; } - - if (!ref->elem) { - SNDERR("error: cannot find '%s' referenced by" - " control '%s'\n", ref->id, elem->id); - return -EINVAL; - } else if (err < 0) - return err; }
return 0; + +ref_missing: + SNDERR("error: cannot find '%s' referenced by control '%s'\n", + ref->id, elem->id); + return -EINVAL; }
static void copy_enum_texts(struct tplg_elem *enum_elem, @@ -183,23 +182,23 @@ static int tplg_build_enum_control(snd_tplg_t *tplg, if (ref->type == SND_TPLG_TYPE_TEXT) { ref->elem = tplg_elem_lookup(&tplg->text_list, ref->id, SND_TPLG_TYPE_TEXT); - if (ref->elem) - copy_enum_texts(elem, ref->elem); + if (!ref->elem) + goto ref_missing;
+ copy_enum_texts(elem, ref->elem); } else if (ref->type == SND_TPLG_TYPE_DATA) { err = tplg_copy_data(tplg, elem, ref); if (err < 0) return err; } - if (!ref->elem) { - SNDERR("error: cannot find '%s' referenced by" - " control '%s'\n", ref->id, elem->id); - return -EINVAL; - } else if (err < 0) - return err; }
return 0; + +ref_missing: + SNDERR("error: cannot find '%s' referenced by control '%s'\n", + ref->id, elem->id); + return -EINVAL; }
/* check referenced private data for a byte control */ diff --git a/src/topology/dapm.c b/src/topology/dapm.c index 4d343b2..988a5fc 100644 --- a/src/topology/dapm.c +++ b/src/topology/dapm.c @@ -160,7 +160,10 @@ static int tplg_build_widget(snd_tplg_t *tplg,
base = &elem->ref_list;
- /* for each ref in this control elem */ + /* Look up and merge each reference in this widget elem. + * For widgets added by C API, its control elements are already set + * and so no need to look up. + */ list_for_each(pos, base) {
ref = list_entry(pos, struct tplg_ref, list); @@ -170,48 +173,50 @@ static int tplg_build_widget(snd_tplg_t *tplg, if (!ref->elem) ref->elem = tplg_elem_lookup(&tplg->mixer_list, ref->id, SND_TPLG_TYPE_MIXER); - if (ref->elem) - err = copy_dapm_control(elem, ref->elem); + if (!ref->elem) + goto ref_missing; + + err = copy_dapm_control(elem, ref->elem); break;
case SND_TPLG_TYPE_ENUM: if (!ref->elem) ref->elem = tplg_elem_lookup(&tplg->enum_list, ref->id, SND_TPLG_TYPE_ENUM); - if (ref->elem) - err = copy_dapm_control(elem, ref->elem); + if (!ref->elem) + goto ref_missing; + + err = copy_dapm_control(elem, ref->elem); break;
case SND_TPLG_TYPE_BYTES: if (!ref->elem) ref->elem = tplg_elem_lookup(&tplg->bytes_ext_list, ref->id, SND_TPLG_TYPE_BYTES); - if (ref->elem) - err = copy_dapm_control(elem, ref->elem); + if (!ref->elem) + goto ref_missing; + + err = copy_dapm_control(elem, ref->elem); break;
case SND_TPLG_TYPE_DATA: err = tplg_copy_data(tplg, elem, ref); - if (err < 0) - return err; break;
default: break; }
- if (!ref->elem) { - SNDERR("error: cannot find control '%s'" - " referenced by widget '%s'\n", - ref->id, elem->id); - return -EINVAL; - } - if (err < 0) return err; }
return 0; + +ref_missing: + SNDERR("error: cannot find '%s' referenced by widget '%s'\n", + ref->id, elem->id); + return -EINVAL; }
int tplg_build_widgets(snd_tplg_t *tplg)
Please overlook this patch. Since v3 has been applied, so this v4 is no longer needed.
Thanks Mengdong
-----Original Message----- From: mengdong.lin@linux.intel.com [mailto:mengdong.lin@linux.intel.com] Sent: Wednesday, July 27, 2016 4:49 PM To: alsa-devel@alsa-project.org Cc: tiwai@suse.de; Lin, Mengdong; o-takashi@sakamocchi.jp; Mengdong Lin Subject: [PATCH v4] topology: Return -EINVAL at once on failure to find a reference
From: Mengdong Lin mengdong.lin@linux.intel.com
In building phase, return error -EINVAL at once if an element's references (e.g. TLV, text, control or private data) cannot be found.
Move this error check right after looking up a reference, to make the code more clear. And checking the return value of tplg_copy_data() is enough, no need to check ref->elem for data references.
Signed-off-by: Mengdong Lin mengdong.lin@linux.intel.com
diff --git a/src/topology/ctl.c b/src/topology/ctl.c index 54f8e44..d910230 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -135,24 +135,23 @@ static int tplg_build_mixer_control(snd_tplg_t *tplg, if (ref->type == SND_TPLG_TYPE_TLV) { ref->elem = tplg_elem_lookup(&tplg->tlv_list, ref->id, SND_TPLG_TYPE_TLV);
if (ref->elem)err = copy_tlv(elem, ref->elem);
if (!ref->elem)goto ref_missing;copy_tlv(elem, ref->elem);} else if (ref->type == SND_TPLG_TYPE_DATA) { err = tplg_copy_data(tplg, elem, ref); if (err < 0) return err; }
if (!ref->elem) {SNDERR("error: cannot find '%s' referenced by"" control '%s'\n", ref->id, elem->id);return -EINVAL;} else if (err < 0)return err;}
return 0;
+ref_missing:
- SNDERR("error: cannot find '%s' referenced by control '%s'\n",
ref->id, elem->id);- return -EINVAL;
}
static void copy_enum_texts(struct tplg_elem *enum_elem, @@ -183,23 +182,23 @@ static int tplg_build_enum_control(snd_tplg_t *tplg, if (ref->type == SND_TPLG_TYPE_TEXT) { ref->elem = tplg_elem_lookup(&tplg->text_list, ref->id, SND_TPLG_TYPE_TEXT);
if (ref->elem)copy_enum_texts(elem, ref->elem);
if (!ref->elem)goto ref_missing;copy_enum_texts(elem, ref->elem);} else if (ref->type == SND_TPLG_TYPE_DATA) { err = tplg_copy_data(tplg, elem, ref); if (err < 0) return err; }
if (!ref->elem) {SNDERR("error: cannot find '%s' referenced by"" control '%s'\n", ref->id, elem->id);return -EINVAL;} else if (err < 0)return err;}
return 0;
+ref_missing:
- SNDERR("error: cannot find '%s' referenced by control '%s'\n",
ref->id, elem->id);- return -EINVAL;
}
/* check referenced private data for a byte control */ diff --git a/src/topology/dapm.c b/src/topology/dapm.c index 4d343b2..988a5fc 100644 --- a/src/topology/dapm.c +++ b/src/topology/dapm.c @@ -160,7 +160,10 @@ static int tplg_build_widget(snd_tplg_t *tplg,
base = &elem->ref_list;
- /* for each ref in this control elem */
/* Look up and merge each reference in this widget elem.
* For widgets added by C API, its control elements are already set* and so no need to look up.*/list_for_each(pos, base) {
ref = list_entry(pos, struct tplg_ref, list); @@ -170,48 +173,50 @@
static int tplg_build_widget(snd_tplg_t *tplg, if (!ref->elem) ref->elem = tplg_elem_lookup(&tplg->mixer_list, ref->id, SND_TPLG_TYPE_MIXER);
if (ref->elem)err = copy_dapm_control(elem, ref->elem);
if (!ref->elem)goto ref_missing;err = copy_dapm_control(elem, ref->elem); break;case SND_TPLG_TYPE_ENUM: if (!ref->elem) ref->elem = tplg_elem_lookup(&tplg->enum_list, ref->id, SND_TPLG_TYPE_ENUM);
if (ref->elem)err = copy_dapm_control(elem, ref->elem);
if (!ref->elem)goto ref_missing;err = copy_dapm_control(elem, ref->elem); break;case SND_TPLG_TYPE_BYTES: if (!ref->elem) ref->elem = tplg_elem_lookup(&tplg->bytes_ext_list, ref->id, SND_TPLG_TYPE_BYTES);
if (ref->elem)err = copy_dapm_control(elem, ref->elem);
if (!ref->elem)goto ref_missing;err = copy_dapm_control(elem, ref->elem); break;case SND_TPLG_TYPE_DATA: err = tplg_copy_data(tplg, elem, ref);
if (err < 0)return err; break;default: break; }
if (!ref->elem) {SNDERR("error: cannot find control '%s'"" referenced by widget '%s'\n",ref->id, elem->id);return -EINVAL;}if (err < 0) return err; }
return 0;
+ref_missing:
- SNDERR("error: cannot find '%s' referenced by widget '%s'\n",
ref->id, elem->id);- return -EINVAL;
}
int tplg_build_widgets(snd_tplg_t *tplg)
2.5.0
Hi,
On Jul 28 2016 10:17, Lin, Mengdong wrote:
Please overlook this patch. Since v3 has been applied, so this v4 is no longer needed.
Although, I prefer this direction than the merged patch.
Anyway, it's within merge window for kernel 4.8 and alsa-lib 1.2.0 (or 1.1.2). It's preferable for us to test current master, seek and fix mistakes. Not to post patchsets for the other purposes to disturb maintainers' work.
Well, this is a call graph related to tplg_copy_data().
snd_tplg_build_file() ->snd_tplg_build() ->tplg_build_integ() (src/topology/data.c) ->tplg_build_manifest_data() * ->tplg_copy_data() (src/topology/ctl.c) ->tplg_build_controls() ->tplg_build_mixer_controls() * ->tplg_copy_data() ->tplg_build_enum_controls() * ->tplg_copy_data() ->tplg_build_bytes_controls() * ->tplg_copy_data() (src/topology/dapm.c) ->tplg_build_widget() * ->tplg_copy_data()
The callers of tplg_copy_data() has similar branchs. The branch consists of: 1. look up an element and assigned it to 'ref' variable 2. copy something in the structure
The difference is element type. In this case, I prefer calling each type-specilized method (lookup and copy) in caller side. In this shape, we can easily know the purpose of each branch, and error handling becomes easier.
Please read this commit. (not compiled and tested yet) It's just to show the concept. https://github.com/takaswie/alsa-lib/commit/ab5d63329e29118307a563024718293d...
Furthermore, these methods are different depending on element type. So it might be possible to assign the type-specialized 'lookup' and 'copy' methods to pointers of structure which represents a certain element with type. In this case, the codes are likely to become more simpler, in my opinion. (If it were C++ code, developers would eager to add new base/derived classes.)
Regards
Takashi Sakamoto
-----Original Message----- From: Takashi Sakamoto [mailto:o-takashi@sakamocchi.jp] Sent: Thursday, July 28, 2016 10:05 PM
On Jul 28 2016 10:17, Lin, Mengdong wrote:
Please overlook this patch. Since v3 has been applied, so this v4 is no longer needed.
Although, I prefer this direction than the merged patch.
Hi Sakamoto-san,
Probably we can do this code refactor later, after the merge window.
Anyway, it's within merge window for kernel 4.8 and alsa-lib 1.2.0 (or 1.1.2). It's preferable for us to test current master, seek and fix mistakes. Not to post patchsets for the other purposes to disturb maintainers' work.
Well, this is a call graph related to tplg_copy_data().
snd_tplg_build_file() ->snd_tplg_build() ->tplg_build_integ() (src/topology/data.c) ->tplg_build_manifest_data()
->tplg_copy_data() (src/topology/ctl.c) ->tplg_build_controls() ->tplg_build_mixer_controls()->tplg_copy_data() ->tplg_build_enum_controls()->tplg_copy_data() ->tplg_build_bytes_controls()->tplg_copy_data() (src/topology/dapm.c) ->tplg_build_widget()->tplg_copy_data()The callers of tplg_copy_data() has similar branchs. The branch consists of:
- look up an element and assigned it to 'ref' variable 2. copy something in
the structure
The difference is element type. In this case, I prefer calling each type-specilized method (lookup and copy) in caller side. In this shape, we can easily know the purpose of each branch, and error handling becomes easier.
Please read this commit. (not compiled and tested yet) It's just to show the concept. https://github.com/takaswie/alsa-lib/commit/ab5d63329e29118307a5630 24718293d3a6abd01
The previous code is almost same as that in your patch. Although private data is now only used by controls and widget, but it will be used in PCM, BE DAIs and links in later patches. This why I merge data look up and copy in one function to save the code. I'll check later if to split it again to keep the unified style.
Furthermore, these methods are different depending on element type. So it might be possible to assign the type-specialized 'lookup' and 'copy' methods to pointers of structure which represents a certain element with type. In this case, the codes are likely to become more simpler, in my opinion. (If it were C++ code, developers would eager to add new base/derived classes.)
I hesitate to add more method pointers atm. Now all generic methods are defined by base elements, such as new/free/look up/private data copy. The derived topology element types are quite different and so we define different non-data reference copy methods, although not as method pointers.
Thanks Mengdong
participants (3)
-
Lin, Mengdong -
mengdong.lin＠linux.intel.com -
Takashi Sakamoto