Re: [PATCH] ucm: add cset-tlv command

[Takashi Iwai <tiwai@suse.de>]

On Tue, 22 Mar 2016 10:10:34 +0100,
Hsin-Yu Chao wrote:
> 
> This patch enables UCM to set a file in TLV format to kcontrol by:
> cset-tlv "name='<kcontrol-name>' <path-to-file>"
> This new 'cset-tlv' command will be used to write audio DSP to
> specific alsa control, where the driver expectes a file in TLV
> format.
> 
> Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>

One problem in this approach is that the provided TLV data file isn't
portable.  Since we deal TLV as int arrays, it's endian-sensitive.
At least, some endian check would be needed.

Some other nitpicks:

> ---
>  src/ucm/main.c      | 80 ++++++++++++++++++++++++++++++++++++++++++++---------
>  src/ucm/parser.c    | 10 +++++++
>  src/ucm/ucm_local.h |  1 +
>  3 files changed, 78 insertions(+), 13 deletions(-)
> 
> diff --git a/src/ucm/main.c b/src/ucm/main.c
> index 7e44603..a4ccb65 100644
> --- a/src/ucm/main.c
> +++ b/src/ucm/main.c
> @@ -161,6 +161,47 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
>  	return 0;
>  }
>  
> +static int tlv_parse(char **res,
> +		     snd_ctl_elem_info_t *info,
> +		     const char *filepath)

The function name is somehow confusing.  It appears as if it parses
TLV.  And what it actually does are two things:
- check snd_ctl_elem_info_is_tlv_writable(),
and
- allocate the whole TLV file.

IMO, snd_ctl_elem_info_is_tlv_writable() check can be in the caller
side, and this function (rename whatever better) can just do allocate
and read the TLV data into a buffer.


> +{
> +	int err = 0;
> +	int fd;
> +	struct stat st;
> +	size_t sz;
> +	ssize_t sz_read;
> +
> +	if (!snd_ctl_elem_info_is_tlv_writable(info)) {
> +		err = -EINVAL;
> +		return err;
> +	}
> +	fd = open(filepath, O_RDONLY);
> +	if (fd < 0) {
> +		err = -errno;
> +		return err;
> +	}
> +	if (stat(filepath, &st) == -1) {
> +		err = -errno;
> +		goto __fail;
> +	}
> +	sz = st.st_size;
> +	*res = malloc(sz);

What if a crazy large file was passed?  A sanity check of the file
size might be good.


thanks,

Takashi