* [PATCH v2 0/3] topology: Support widgets' stream name and file inclusion for text conf file
@ 2016-10-20 5:46 mengdong.lin
2016-10-20 5:48 ` [PATCH v2 1/3] topology: Fix missing stream name of widgets in " mengdong.lin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: mengdong.lin @ 2016-10-20 5:46 UTC (permalink / raw)
To: alsa-devel
Cc: Mengdong Lin, tiwai, hardik.t.shah, guneshwor.o.singh,
liam.r.girdwood, vinod.koul, broonie, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/3] topology: Fix missing stream name of widgets in text conf file
2016-10-20 5:46 [PATCH v2 0/3] topology: Support widgets' stream name and file inclusion for text conf file mengdong.lin
@ 2016-10-20 5:48 ` mengdong.lin
2016-10-20 5:48 ` [PATCH v2 2/3] alsaconf: Search included files under global configuration directories mengdong.lin
2016-10-20 5:48 ` [PATCH v2 3/3] topology: Add doc for including other files in the text conf file mengdong.lin
2 siblings, 0 replies; 6+ messages in thread
From: mengdong.lin @ 2016-10-20 5:48 UTC (permalink / raw)
To: alsa-devel
Cc: Mengdong Lin, tiwai, hardik.t.shah, guneshwor.o.singh,
liam.r.girdwood, vinod.koul, broonie, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/3] alsaconf: Search included files under global configuration directories
2016-10-20 5:46 [PATCH v2 0/3] topology: Support widgets' stream name and file inclusion for text conf file mengdong.lin
2016-10-20 5:48 ` [PATCH v2 1/3] topology: Fix missing stream name of widgets in " mengdong.lin
@ 2016-10-20 5:48 ` mengdong.lin
2016-10-21 15:04 ` Takashi Iwai
2016-10-20 5:48 ` [PATCH v2 3/3] topology: Add doc for including other files in the text conf file mengdong.lin
2 siblings, 1 reply; 6+ messages in thread
From: mengdong.lin @ 2016-10-20 5:48 UTC (permalink / raw)
To: alsa-devel
Cc: Mengdong Lin, tiwai, hardik.t.shah, guneshwor.o.singh,
liam.r.girdwood, vinod.koul, broonie, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] topology: Add doc for including other files in the text conf file
2016-10-20 5:46 [PATCH v2 0/3] topology: Support widgets' stream name and file inclusion for text conf file mengdong.lin
2016-10-20 5:48 ` [PATCH v2 1/3] topology: Fix missing stream name of widgets in " mengdong.lin
2016-10-20 5:48 ` [PATCH v2 2/3] alsaconf: Search included files under global configuration directories mengdong.lin
@ 2016-10-20 5:48 ` mengdong.lin
2 siblings, 0 replies; 6+ messages in thread
From: mengdong.lin @ 2016-10-20 5:48 UTC (permalink / raw)
To: alsa-devel
Cc: Mengdong Lin, tiwai, hardik.t.shah, guneshwor.o.singh,
liam.r.girdwood, vinod.koul, broonie, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/3] alsaconf: Search included files under global configuration directories
2016-10-20 5:48 ` [PATCH v2 2/3] alsaconf: Search included files under global configuration directories mengdong.lin
@ 2016-10-21 15:04 ` Takashi Iwai
2016-10-24 13:23 ` Mengdong Lin
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2016-10-21 15:04 UTC (permalink / raw)
To: mengdong.lin
Cc: alsa-devel, vinod.koul, hardik.t.shah, guneshwor.o.singh,
liam.r.girdwood, broonie, mengdong.lin
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/3] alsaconf: Search included files under global configuration directories
2016-10-21 15:04 ` Takashi Iwai
@ 2016-10-24 13:23 ` Mengdong Lin
0 siblings, 0 replies; 6+ messages in thread
From: Mengdong Lin @ 2016-10-24 13:23 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, vinod.koul, hardik.t.shah, guneshwor.o.singh,
liam.r.girdwood, broonie, 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
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-24 13:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-20 5:46 [PATCH v2 0/3] topology: Support widgets' stream name and file inclusion for text conf file mengdong.lin
2016-10-20 5:48 ` [PATCH v2 1/3] topology: Fix missing stream name of widgets in " mengdong.lin
2016-10-20 5:48 ` [PATCH v2 2/3] alsaconf: Search included files under global configuration directories mengdong.lin
2016-10-21 15:04 ` Takashi Iwai
2016-10-24 13:23 ` Mengdong Lin
2016-10-20 5:48 ` [PATCH v2 3/3] topology: Add doc for including other files in the text conf file mengdong.lin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.