[alsa-devel] [PATCH] ucm: add cset-tlv command

[Hsin-yu Chao]

On Tue, Mar 22, 2016 at 7:48 PM, Takashi Iwai <tiwai at suse.de> 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 at 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.
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 at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel