* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).