[PATCH v2 0/3] topology: Support widgets' stream name and file inclusion for text conf file

[PATCH v2 0/3] topology: Support widgets' stream name and file inclusion for text conf file

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

This series doesn't require or cause any ABI updates. It doesn't need the
kernel patches that trying to udpate ABI from v4 or v5.

Patch 1 fixes a missing topology feature for text conf file to configure
	stream name of a widget.

Patch 2 allows alsaconf to search included files under global
        configuration directories. This will benefit topology and UCM.

Patch 3 adds document about how to include files for topology
	configuration. There is no c code change but actually it depends
	on patch 2.

History:
v2: 	No longer use topology-specific code to support searching included
	files, but extend alsaconf to support this and user can just use
	alsaconf syntax <xxx> and <confdir:xxx>. 

Mengdong Lin (3):
  topology: Fix missing stream name of widgets in text conf file
  alsaconf: Search included files under global configuration directories
  topology: Add doc for including other files in the text conf file

 include/topology.h  |  31 ++++++++++++
 src/conf.c          | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/topology/dapm.c |  10 ++++
 3 files changed, 180 insertions(+), 2 deletions(-)

-- 
2.5.0

[PATCH v2 1/3] topology: Fix missing stream name of widgets in text conf file

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

User can define the stream name of an input/output widget in the text conf
file, by setting "stream_name" of a SectionWidget.

Topology C API and kernel already have support for configuring a widget's
stream name. This patch just adds the missing part of the text conf file.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/include/topology.h b/include/topology.h
index 0675b52..a0d018e 100644
--- a/include/topology.h
+++ b/include/topology.h
@@ -483,6 +483,7 @@ extern "C" {
  *	index "1"			# Index number
  *
  *	type "aif_in"			# Widget type - detailed above
+ *	stream_name "name"		# Stream name
  *
  *	no_pm "true"			# No PM control bit.
  *	reg "20"			# PM bit register offset
diff --git a/src/topology/dapm.c b/src/topology/dapm.c
index e3c90d8..9fa0aac 100644
--- a/src/topology/dapm.c
+++ b/src/topology/dapm.c
@@ -507,6 +507,16 @@ int tplg_parse_dapm_widget(snd_tplg_t *tplg,
 			continue;
 		}
 
+		if (strcmp(id, "stream_name") == 0) {
+			if (snd_config_get_string(n, &val) < 0)
+				return -EINVAL;
+
+			elem_copy_text(widget->sname, val,
+				       SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
+			tplg_dbg("\t%s: %s\n", id, val);
+			continue;
+		}
+
 		if (strcmp(id, "no_pm") == 0) {
 			if (snd_config_get_string(n, &val) < 0)
 				return -EINVAL;
-- 
2.5.0

[PATCH v2 2/3] alsaconf: Search included files under global configuration directories

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

Users can include file by giving a relative path or just a file name via
alsaconf syntax <xxx>. ALSA conf will search the file in top configuration
directory and additional configuration directories defined by users via
alsaconf syntax <confdir:/xxx>.

ALSA conf will search and open an included file in the following order
of priority:

1. directly open the file by its name;
2. search for the file name in top config direcotry "usr/share/alsa/";
3. search for the file name in additional configuration directories
   specified by users, via alsaconf syntax
   <confdir:/relative-path/to/user/share/alsa>;

The configuration directories defined by a file will only be used to search
the files included by this file.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/src/conf.c b/src/conf.c
index a516611..44ef673 100644
--- a/src/conf.c
+++ b/src/conf.c
@@ -456,6 +456,18 @@ struct filedesc {
 	snd_input_t *in;
 	unsigned int line, column;
 	struct filedesc *next;
+
+	/* list of the include paths (configuration directories),
+	 * defined by <confdir:/relative-path/to/top-alsa-conf-dir>,
+	 * for searching its included files.
+	 */
+	struct list_head include_paths;
+};
+
+/* path to search included files */
+struct include_path {
+	char *dir;
+	struct list_head list;
 };
 
 #define LOCAL_ERROR			(-0x68000000)
@@ -501,6 +513,120 @@ static inline void snd_config_unlock(void) { }
 
 #endif
 
+/**
+ * \brief Add a diretory to the paths to search included files.
+ * \param fd File object that owns these paths to search files included by it.
+ * \param dir Path of the directory to add.
+ * \return Zero if successful, otherwise a negative error code.
+ *
+ * The direcotry should be a subdiretory of top configuration directory
+ * "/usr/share/alsa/".
+ */
+static int add_include_path(struct filedesc *fd, char *dir)
+{
+	struct include_path *path;
+
+	path = calloc(1, sizeof(*path));
+	if (!path)
+		return -ENOMEM;
+
+	path->dir = calloc(1, PATH_MAX + 1);
+	if (!path->dir)
+		return -ENOMEM;
+
+	strncpy(path->dir, dir, PATH_MAX);
+	list_add_tail(&path->list, &fd->include_paths);
+	return 0;
+}
+
+/**
+ * \brief Free all include paths of a file descriptor.
+ * \param fd File object that owns these paths to search files included by it.
+ */
+static void free_include_paths(struct filedesc *fd)
+{
+	struct list_head *pos, *npos, *base;
+	struct include_path *path;
+
+	base = &fd->include_paths;
+	list_for_each_safe(pos, npos, base) {
+		path = list_entry(pos, struct include_path, list);
+		list_del(&path->list);
+		if (path->dir)
+			free(path->dir);
+		free(path);
+	}
+}
+
+/**
+ * \brief Search and open a file, and creates a new input object reading
+ *        from the file.
+ * \param inputp The functions puts the pointer to the new input object
+ *               at the address specified by \p inputp.
+ * \param file Name of the configuration file.
+ * \param include_paths Optional, addtional directories to search the file.
+ * \return Zero if successful, otherwise a negative error code.
+ *
+ * This function will search and open the file in the following order
+ * of priority:
+ * 1. directly open the file by its name;
+ * 2. search for the file name in top configuration directory
+ *     "usr/share/alsa/";
+ * 3. search for the file name in in additional configuration directories
+ *     specified by users, via alsaconf syntax
+ *     <confdir:/relative-path/to/user/share/alsa>;
+ *     These directories should be subdirectories of /usr/share/alsa.
+ */
+static int input_stdio_open(snd_input_t **inputp, const char *file,
+			    struct list_head *include_paths)
+{
+	struct list_head *pos, *npos, *base;
+	struct include_path *path;
+	char *full_path = NULL;
+	int err = 0;
+
+	err = snd_input_stdio_open(inputp, file, "r");
+	if (err == 0)
+		goto out;
+
+	full_path = calloc(1, PATH_MAX + 1);
+	if (!full_path)
+		return -ENOMEM;
+
+	/* search file in top configuration directory usr/share/alsa */
+	strcpy(full_path, ALSA_CONFIG_DIR);
+	strcat(full_path, "/");
+	strcat(full_path, file);
+	err = snd_input_stdio_open(inputp, full_path, "r");
+	if (err == 0)
+		goto out;
+
+	/* search file in user specified include paths. These directories
+	 * are subdirectories of /usr/share/alsa.
+	 */
+	if (include_paths) {
+		base = include_paths;
+		list_for_each_safe(pos, npos, base) {
+			path = list_entry(pos, struct include_path, list);
+			if (!path->dir)
+				continue;
+
+			strncpy(full_path, path->dir, PATH_MAX);
+			strcat(full_path, "/");
+			strcat(full_path, file);
+			err = snd_input_stdio_open(inputp, full_path, "r");
+			if (err == 0)
+			goto out;
+		}
+	}
+
+out:
+	if (full_path)
+		free(full_path);
+
+	return err;
+}
+
 static int safe_strtoll(const char *str, long long *val)
 {
 	long long v;
@@ -632,7 +758,7 @@ static int get_char_skip_comments(input_t *input)
 			int err = get_delimstring(&str, '>', input);
 			if (err < 0)
 				return err;
-			if (!strncmp(str, "confdir:", 8)) {
+			if (!strncmp(str, "confdir:", 8)) { /* directory */
 				char *tmp = malloc(strlen(ALSA_CONFIG_DIR) + 1 + strlen(str + 8) + 1);
 				if (tmp == NULL) {
 					free(str);
@@ -641,8 +767,14 @@ static int get_char_skip_comments(input_t *input)
 				sprintf(tmp, ALSA_CONFIG_DIR "/%s", str + 8);
 				free(str);
 				str = tmp;
+
+				err = snd_input_stdio_open(&in, str, "r");
+				if (err == 0)
+					add_include_path(input->current, str);
+			} else { /* file, also search in include paths */
+				err = input_stdio_open(&in, str,
+						&input->current->include_paths);
 			}
-			err = snd_input_stdio_open(&in, str, "r");
 			if (err < 0) {
 				SNDERR("Cannot access file %s", str);
 				free(str);
@@ -658,6 +790,7 @@ static int get_char_skip_comments(input_t *input)
 			fd->next = input->current;
 			fd->line = 1;
 			fd->column = 0;
+			INIT_LIST_HEAD(&fd->include_paths);
 			input->current = fd;
 			continue;
 		}
@@ -1668,6 +1801,7 @@ static int snd_config_load1(snd_config_t *config, snd_input_t *in, int override)
 	fd->line = 1;
 	fd->column = 0;
 	fd->next = NULL;
+	INIT_LIST_HEAD(&fd->include_paths);
 	input.current = fd;
 	input.unget = 0;
 	err = parse_defs(config, &input, 0, override);
@@ -1708,9 +1842,12 @@ static int snd_config_load1(snd_config_t *config, snd_input_t *in, int override)
 		fd_next = fd->next;
 		snd_input_close(fd->in);
 		free(fd->name);
+		free_include_paths(fd);
 		free(fd);
 		fd = fd_next;
 	}
+
+	free_include_paths(fd);
 	free(fd);
 	return err;
 }
-- 
2.5.0

Re: [PATCH v2 2/3] alsaconf: Search included files under global configuration directories

[Takashi Iwai]

On Thu, 20 Oct 2016 07:48:37 +0200,
mengdong.lin@linux.intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> Users can include file by giving a relative path or just a file name via
> alsaconf syntax <xxx>. ALSA conf will search the file in top configuration
> directory and additional configuration directories defined by users via
> alsaconf syntax <confdir:/xxx>.
> 
> ALSA conf will search and open an included file in the following order
> of priority:
> 
> 1. directly open the file by its name;
> 2. search for the file name in top config direcotry "usr/share/alsa/";
> 3. search for the file name in additional configuration directories
>    specified by users, via alsaconf syntax
>    <confdir:/relative-path/to/user/share/alsa>;

This would break the existing use case of <confdir:xxx>.
Currently, "confdir:" simply replaces itself with the default config
directory.  For example, <confdir:foo/bar> will include the file
/usr/share/alsa/foo/bar, and it doesn't mean to specify the
directory.  You can think of "confdir:" as $ALSA_CONF_DIR.

In your use case, you want to enumerate directories.  Then either
create a new macro (e.g. "searchdir") or check properly whether the
given path is a directory.


> The configuration directories defined by a file will only be used to search
> the files included by this file.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> diff --git a/src/conf.c b/src/conf.c
> index a516611..44ef673 100644
> --- a/src/conf.c
> +++ b/src/conf.c
> @@ -456,6 +456,18 @@ struct filedesc {
>  	snd_input_t *in;
>  	unsigned int line, column;
>  	struct filedesc *next;
> +
> +	/* list of the include paths (configuration directories),
> +	 * defined by <confdir:/relative-path/to/top-alsa-conf-dir>,
> +	 * for searching its included files.
> +	 */
> +	struct list_head include_paths;
> +};
> +
> +/* path to search included files */
> +struct include_path {
> +	char *dir;
> +	struct list_head list;
>  };
>  
>  #define LOCAL_ERROR			(-0x68000000)
> @@ -501,6 +513,120 @@ static inline void snd_config_unlock(void) { }
>  
>  #endif
>  
> +/**
> + * \brief Add a diretory to the paths to search included files.
> + * \param fd File object that owns these paths to search files included by it.
> + * \param dir Path of the directory to add.
> + * \return Zero if successful, otherwise a negative error code.
> + *
> + * The direcotry should be a subdiretory of top configuration directory
> + * "/usr/share/alsa/".

Don't use the doxygen comment for static functions.
They shouldn't be included in the exported documents.


> + */
> +static int add_include_path(struct filedesc *fd, char *dir)
> +{
> +	struct include_path *path;
> +
> +	path = calloc(1, sizeof(*path));
> +	if (!path)
> +		return -ENOMEM;
> +
> +	path->dir = calloc(1, PATH_MAX + 1);
> +	if (!path->dir)
> +		return -ENOMEM;

Leaking the path object here.

> +	strncpy(path->dir, dir, PATH_MAX);

Isn't the dir the proper NUL-terminated string?  If so, we can simply
copy like:
	path->dir = strdup(dir);
	if (!path->dir) {
		...


> +	list_add_tail(&path->list, &fd->include_paths);
> +	return 0;
> +}
> +
> +/**
> + * \brief Free all include paths of a file descriptor.
> + * \param fd File object that owns these paths to search files included by it.
> + */

Ditto, no doxygen comments.


> +static void free_include_paths(struct filedesc *fd)
> +{
> +	struct list_head *pos, *npos, *base;
> +	struct include_path *path;
> +
> +	base = &fd->include_paths;
> +	list_for_each_safe(pos, npos, base) {
> +		path = list_entry(pos, struct include_path, list);
> +		list_del(&path->list);
> +		if (path->dir)
> +			free(path->dir);
> +		free(path);
> +	}
> +}
> +
> +/**
> + * \brief Search and open a file, and creates a new input object reading
> + *        from the file.
> + * \param inputp The functions puts the pointer to the new input object
> + *               at the address specified by \p inputp.
> + * \param file Name of the configuration file.
> + * \param include_paths Optional, addtional directories to search the file.
> + * \return Zero if successful, otherwise a negative error code.
> + *
> + * This function will search and open the file in the following order
> + * of priority:
> + * 1. directly open the file by its name;
> + * 2. search for the file name in top configuration directory
> + *     "usr/share/alsa/";
> + * 3. search for the file name in in additional configuration directories
> + *     specified by users, via alsaconf syntax
> + *     <confdir:/relative-path/to/user/share/alsa>;
> + *     These directories should be subdirectories of /usr/share/alsa.
> + */

Ditto.

> +static int input_stdio_open(snd_input_t **inputp, const char *file,
> +			    struct list_head *include_paths)
> +{
> +	struct list_head *pos, *npos, *base;
> +	struct include_path *path;
> +	char *full_path = NULL;
> +	int err = 0;
> +
> +	err = snd_input_stdio_open(inputp, file, "r");
> +	if (err == 0)
> +		goto out;
> +
> +	full_path = calloc(1, PATH_MAX + 1);
> +	if (!full_path)
> +		return -ENOMEM;

In user-space we have a bigger stack, so you can just use the auto
variable here instead of heap.  It's more efficient.

> +
> +	/* search file in top configuration directory usr/share/alsa */
> +	strcpy(full_path, ALSA_CONFIG_DIR);
> +	strcat(full_path, "/");
> +	strcat(full_path, file);

This may overflow.  Do like
	snprintf(full_path, sizeof(full_path), "%s/%s",
		ALSA_CONFIG_DIR, file);


> +	err = snd_input_stdio_open(inputp, full_path, "r");
> +	if (err == 0)
> +		goto out;
> +
> +	/* search file in user specified include paths. These directories
> +	 * are subdirectories of /usr/share/alsa.
> +	 */
> +	if (include_paths) {
> +		base = include_paths;
> +		list_for_each_safe(pos, npos, base) {

No need to use *_safe version here, as we don't remove the list entry.

> +			path = list_entry(pos, struct include_path, list);
> +			if (!path->dir)
> +				continue;
> +
> +			strncpy(full_path, path->dir, PATH_MAX);
> +			strcat(full_path, "/");
> +			strcat(full_path, file);
> +			err = snd_input_stdio_open(inputp, full_path, "r");
> +			if (err == 0)
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	if (full_path)
> +		free(full_path);
> +
> +	return err;
> +}
> +
>  static int safe_strtoll(const char *str, long long *val)
>  {
>  	long long v;
> @@ -632,7 +758,7 @@ static int get_char_skip_comments(input_t *input)
>  			int err = get_delimstring(&str, '>', input);
>  			if (err < 0)
>  				return err;
> -			if (!strncmp(str, "confdir:", 8)) {
> +			if (!strncmp(str, "confdir:", 8)) { /* directory */
>  				char *tmp = malloc(strlen(ALSA_CONFIG_DIR) + 1 + strlen(str + 8) + 1);
>  				if (tmp == NULL) {
>  					free(str);
> @@ -641,8 +767,14 @@ static int get_char_skip_comments(input_t *input)
>  				sprintf(tmp, ALSA_CONFIG_DIR "/%s", str + 8);
>  				free(str);
>  				str = tmp;
> +
> +				err = snd_input_stdio_open(&in, str, "r");
> +				if (err == 0)
> +					add_include_path(input->current, str);

See my comment above.


thanks,

Takashi


> +			} else { /* file, also search in include paths */
> +				err = input_stdio_open(&in, str,
> +						&input->current->include_paths);
>  			}
> -			err = snd_input_stdio_open(&in, str, "r");
>  			if (err < 0) {
>  				SNDERR("Cannot access file %s", str);
>  				free(str);
> @@ -658,6 +790,7 @@ static int get_char_skip_comments(input_t *input)
>  			fd->next = input->current;
>  			fd->line = 1;
>  			fd->column = 0;
> +			INIT_LIST_HEAD(&fd->include_paths);
>  			input->current = fd;
>  			continue;
>  		}
> @@ -1668,6 +1801,7 @@ static int snd_config_load1(snd_config_t *config, snd_input_t *in, int override)
>  	fd->line = 1;
>  	fd->column = 0;
>  	fd->next = NULL;
> +	INIT_LIST_HEAD(&fd->include_paths);
>  	input.current = fd;
>  	input.unget = 0;
>  	err = parse_defs(config, &input, 0, override);
> @@ -1708,9 +1842,12 @@ static int snd_config_load1(snd_config_t *config, snd_input_t *in, int override)
>  		fd_next = fd->next;
>  		snd_input_close(fd->in);
>  		free(fd->name);
> +		free_include_paths(fd);
>  		free(fd);
>  		fd = fd_next;
>  	}
> +
> +	free_include_paths(fd);
>  	free(fd);
>  	return err;
>  }
> -- 
> 2.5.0
> 

Re: [PATCH v2 2/3] alsaconf: Search included files under global configuration directories

[Mengdong Lin]

On 10/21/2016 11:04 PM, Takashi Iwai wrote:
> On Thu, 20 Oct 2016 07:48:37 +0200,
> mengdong.lin@linux.intel.com wrote:
>>
>> From: Mengdong Lin <mengdong.lin@linux.intel.com>
>>
>> Users can include file by giving a relative path or just a file name via
>> alsaconf syntax <xxx>. ALSA conf will search the file in top configuration
>> directory and additional configuration directories defined by users via
>> alsaconf syntax <confdir:/xxx>.
>>
>> ALSA conf will search and open an included file in the following order
>> of priority:
>>
>> 1. directly open the file by its name;
>> 2. search for the file name in top config direcotry "usr/share/alsa/";
>> 3. search for the file name in additional configuration directories
>>     specified by users, via alsaconf syntax
>>     <confdir:/relative-path/to/user/share/alsa>;
>
> This would break the existing use case of <confdir:xxx>.
> Currently, "confdir:" simply replaces itself with the default config
> directory.  For example, <confdir:foo/bar> will include the file
> /usr/share/alsa/foo/bar, and it doesn't mean to specify the
> directory.  You can think of "confdir:" as $ALSA_CONF_DIR.
>
> In your use case, you want to enumerate directories.  Then either
> create a new macro (e.g. "searchdir") or check properly whether the
> given path is a directory.

Thanks for pointing out the mistake.
I'll use <searchdir:xxx> and only files with a relative path
will be searched under <usr/share/alsa> and user specified directories 
via syntax <searchdir:xxx>.

>
>
>> The configuration directories defined by a file will only be used to search
>> the files included by this file.
>>
>> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
>>
>> diff --git a/src/conf.c b/src/conf.c
>> index a516611..44ef673 100644
>> --- a/src/conf.c
>> +++ b/src/conf.c
>> @@ -456,6 +456,18 @@ struct filedesc {
>>   	snd_input_t *in;
>>   	unsigned int line, column;
>>   	struct filedesc *next;
>> +
>> +	/* list of the include paths (configuration directories),
>> +	 * defined by <confdir:/relative-path/to/top-alsa-conf-dir>,
>> +	 * for searching its included files.
>> +	 */
>> +	struct list_head include_paths;
>> +};
>> +
>> +/* path to search included files */
>> +struct include_path {
>> +	char *dir;
>> +	struct list_head list;
>>   };
>>
>>   #define LOCAL_ERROR			(-0x68000000)
>> @@ -501,6 +513,120 @@ static inline void snd_config_unlock(void) { }
>>
>>   #endif
>>
>> +/**
>> + * \brief Add a diretory to the paths to search included files.
>> + * \param fd File object that owns these paths to search files included by it.
>> + * \param dir Path of the directory to add.
>> + * \return Zero if successful, otherwise a negative error code.
>> + *
>> + * The direcotry should be a subdiretory of top configuration directory
>> + * "/usr/share/alsa/".
>
> Don't use the doxygen comment for static functions.
> They shouldn't be included in the exported documents.
>
>

Okay. I'll change the format.

>> + */
>> +static int add_include_path(struct filedesc *fd, char *dir)
>> +{
>> +	struct include_path *path;
>> +
>> +	path = calloc(1, sizeof(*path));
>> +	if (!path)
>> +		return -ENOMEM;
>> +
>> +	path->dir = calloc(1, PATH_MAX + 1);
>> +	if (!path->dir)
>> +		return -ENOMEM;
>
> Leaking the path object here.
>

Sorry, I'll fix this. New code will have dir allocated by the caller, so 
here we'll just store the string pointer and there will be no calloc and 
free on error here.

>> +	strncpy(path->dir, dir, PATH_MAX);
>
> Isn't the dir the proper NUL-terminated string?  If so, we can simply
> copy like:
> 	path->dir = strdup(dir);
> 	if (!path->dir) {
> 		...
>

Yes.

>> +	list_add_tail(&path->list, &fd->include_paths);
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \brief Free all include paths of a file descriptor.
>> + * \param fd File object that owns these paths to search files included by it.
>> + */
>
> Ditto, no doxygen comments.

Okay.

>
>> +static void free_include_paths(struct filedesc *fd)
>> +{
>> +	struct list_head *pos, *npos, *base;
>> +	struct include_path *path;
>> +
>> +	base = &fd->include_paths;
>> +	list_for_each_safe(pos, npos, base) {
>> +		path = list_entry(pos, struct include_path, list);
>> +		list_del(&path->list);
>> +		if (path->dir)
>> +			free(path->dir);
>> +		free(path);
>> +	}
>> +}
>> +
>> +/**
>> + * \brief Search and open a file, and creates a new input object reading
>> + *        from the file.
>> + * \param inputp The functions puts the pointer to the new input object
>> + *               at the address specified by \p inputp.
>> + * \param file Name of the configuration file.
>> + * \param include_paths Optional, addtional directories to search the file.
>> + * \return Zero if successful, otherwise a negative error code.
>> + *
>> + * This function will search and open the file in the following order
>> + * of priority:
>> + * 1. directly open the file by its name;
>> + * 2. search for the file name in top configuration directory
>> + *     "usr/share/alsa/";
>> + * 3. search for the file name in in additional configuration directories
>> + *     specified by users, via alsaconf syntax
>> + *     <confdir:/relative-path/to/user/share/alsa>;
>> + *     These directories should be subdirectories of /usr/share/alsa.
>> + */
>
> Ditto.

Okay.

>> +static int input_stdio_open(snd_input_t **inputp, const char *file,
>> +			    struct list_head *include_paths)
>> +{
>> +	struct list_head *pos, *npos, *base;
>> +	struct include_path *path;
>> +	char *full_path = NULL;
>> +	int err = 0;
>> +
>> +	err = snd_input_stdio_open(inputp, file, "r");
>> +	if (err == 0)
>> +		goto out;
>> +
>> +	full_path = calloc(1, PATH_MAX + 1);
>> +	if (!full_path)
>> +		return -ENOMEM;
>
> In user-space we have a bigger stack, so you can just use the auto
> variable here instead of heap.  It's more efficient.

Okay, I'll do this. Thanks for the tips :-)

>
>> +
>> +	/* search file in top configuration directory usr/share/alsa */
>> +	strcpy(full_path, ALSA_CONFIG_DIR);
>> +	strcat(full_path, "/");
>> +	strcat(full_path, file);
>
> This may overflow.  Do like
> 	snprintf(full_path, sizeof(full_path), "%s/%s",
> 		ALSA_CONFIG_DIR, file);
>

Yes, there is the risk and snprintf looks nice. I'll fix as you suggested.


>> +	err = snd_input_stdio_open(inputp, full_path, "r");
>> +	if (err == 0)
>> +		goto out;
>> +
>> +	/* search file in user specified include paths. These directories
>> +	 * are subdirectories of /usr/share/alsa.
>> +	 */
>> +	if (include_paths) {
>> +		base = include_paths;
>> +		list_for_each_safe(pos, npos, base) {
>
> No need to use *_safe version here, as we don't remove the list entry.

Okay. And I'll also use snprintf here.

>
>> +			path = list_entry(pos, struct include_path, list);
>> +			if (!path->dir)
>> +				continue;
>> +
>> +			strncpy(full_path, path->dir, PATH_MAX);
>> +			strcat(full_path, "/");
>> +			strcat(full_path, file);
>> +			err = snd_input_stdio_open(inputp, full_path, "r");
>> +			if (err == 0)
>> +			goto out;
>> +		}
>> +	}
>> +
>> +out:
>> +	if (full_path)
>> +		free(full_path);
>> +
>> +	return err;
>> +}
>> +
>>   static int safe_strtoll(const char *str, long long *val)
>>   {
>>   	long long v;
>> @@ -632,7 +758,7 @@ static int get_char_skip_comments(input_t *input)
>>   			int err = get_delimstring(&str, '>', input);
>>   			if (err < 0)
>>   				return err;
>> -			if (!strncmp(str, "confdir:", 8)) {
>> +			if (!strncmp(str, "confdir:", 8)) { /* directory */
>>   				char *tmp = malloc(strlen(ALSA_CONFIG_DIR) + 1 + strlen(str + 8) + 1);
>>   				if (tmp == NULL) {
>>   					free(str);
>> @@ -641,8 +767,14 @@ static int get_char_skip_comments(input_t *input)
>>   				sprintf(tmp, ALSA_CONFIG_DIR "/%s", str + 8);
>>   				free(str);
>>   				str = tmp;
>> +
>> +				err = snd_input_stdio_open(&in, str, "r");
>> +				if (err == 0)
>> +					add_include_path(input->current, str);
>
> See my comment above.
>
>
> thanks,
>
> Takashi

Thank you very much for the review and comments!
I've fixed the issues in the series v4. Please review.

Mengdong


>
>
>> +			} else { /* file, also search in include paths */
>> +				err = input_stdio_open(&in, str,
>> +						&input->current->include_paths);
>>   			}
>> -			err = snd_input_stdio_open(&in, str, "r");
>>   			if (err < 0) {
>>   				SNDERR("Cannot access file %s", str);
>>   				free(str);
>> @@ -658,6 +790,7 @@ static int get_char_skip_comments(input_t *input)
>>   			fd->next = input->current;
>>   			fd->line = 1;
>>   			fd->column = 0;
>> +			INIT_LIST_HEAD(&fd->include_paths);
>>   			input->current = fd;
>>   			continue;
>>   		}
>> @@ -1668,6 +1801,7 @@ static int snd_config_load1(snd_config_t *config, snd_input_t *in, int override)
>>   	fd->line = 1;
>>   	fd->column = 0;
>>   	fd->next = NULL;
>> +	INIT_LIST_HEAD(&fd->include_paths);
>>   	input.current = fd;
>>   	input.unget = 0;
>>   	err = parse_defs(config, &input, 0, override);
>> @@ -1708,9 +1842,12 @@ static int snd_config_load1(snd_config_t *config, snd_input_t *in, int override)
>>   		fd_next = fd->next;
>>   		snd_input_close(fd->in);
>>   		free(fd->name);
>> +		free_include_paths(fd);
>>   		free(fd);
>>   		fd = fd_next;
>>   	}
>> +
>> +	free_include_paths(fd);
>>   	free(fd);
>>   	return err;
>>   }
>> --
>> 2.5.0
>>
>

[PATCH v2 3/3] topology: Add doc for including other files in the text conf file

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

This patch adds document about how to include other files in the text
configuration files, by alsaconf syntax <xxx> and <confdir:xxx>.

Users may define common info in separate files (e.g. vendor tokens,
tuples) and share them for different platforms, by including them via
syntax <path/to/configuration-file>. This can save the total size of
files. Users can also specifiy additional configuraiton directories
relative to "usr/share/alsa/" to search the included files, via syntax
<confdir:/relative-path/to/usr/share/alsa>.

The alsaconf will search and open an included file in the following order
of priority:
1. directly open the file by its name;
2. search for the file name in "usr/share/alsa";
3. search for the file name in user specified subdirectories under
   "usr/share/alsa".

The order of the included files need not to be same as their dependencies,
because the toplogy library will load all of them before parsing their
dependencies.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/include/topology.h b/include/topology.h
index a0d018e..c3449fd 100644
--- a/include/topology.h
+++ b/include/topology.h
@@ -624,6 +624,36 @@ extern "C" {
  *	data "name"			# optional private data
  * }
  * </pre>
+ *
+ * <h4>Include other files</h4>
+ * Users may include other files in a text conf file via alsaconf syntax
+ * <path/to/configuration-file>. This allows users to define common info
+ * in separate files (e.g. vendor tokens, tuples) and share them for
+ * different platforms, thus save the total size of config files. <br>
+ * Users can also specifiy additional configuraiton directories relative
+ * to "usr/share/alsa/" to search the included files,  via alsaconf syntax
+ * <confdir:/relative-path/to/usr/share/alsa>. <br><br>
+ *
+ * For example, file A and file B are two text conf files for platform X,
+ * they will be installed to /usr/share/alsa/topology/platformx. If we
+ * need file A to include file B, in file A we can add: <br>
+ *
+ * <confdir:topology/platformx> <br>
+ * <name-of-file-B> <br><br>
+ *
+ * ALSA conf will search and open an included file in the following order
+ * of priority:
+ *  1. directly open the file by its name;
+ *  2. search for the file name in "usr/share/alsa";
+ *  3. search for the file name in user specified subdirectories under
+ *     "usr/share/alsa".
+ *
+ * The order of the included files need not to be same as their
+ * dependencies, since the topology library will load them all before
+ * parsing their dependencies. <br>
+ *
+ * The configuration directories defined by a file will only be used to search
+ * the files included by this file.
  */
 
 /** Maximum number of channels supported in one control */
-- 
2.5.0