On Tue, Mar 22, 2016 at 7:48 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> 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@xxxxxxxxxxxx>
>
> 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.
Thanks for the review. I think by extracting the length attribute (i.e
tlv[1]) and compare it with the file size is sufficient here, since
there is no way to figure out the endianness of the binary file passed
in.
Also I agree that additional check for crazy large or small file is
necessary. According to the dsp files I test with, I'll set the tlv
file size limit to between 8 bytes and 1MB.
Will upload a v2 patch shortly, thanks!
Hsin-yu
>
> 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
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
