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

[Mengdong Lin <mengdong.lin@linux.intel.com>]

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
>>
>