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

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