diff mbox

[v4] topology: Return -EINVAL at once on failure to find a reference

Message ID 1469609333-13961-1-git-send-email-mengdong.lin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

mengdong.lin@linux.intel.com July 27, 2016, 8:48 a.m. UTC 
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>

Comments

Lin, Mengdong July 28, 2016, 1:17 a.m. UTC | #1 
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
Takashi Sakamoto July 28, 2016, 2:05 p.m. UTC | #2 
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/ab5d63329e29118307a563024718293d3a6abd01

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
Lin, Mengdong July 28, 2016, 3:22 p.m. UTC | #3 
> -----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:
> 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/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
diff mbox

Patch

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)