Re: [PATCH v2 2/2] topology: Fix the missing referenced elem ptr when merging private data

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

Hi,

I submitted a new v4 patch to solve this issue, which will return 
-EINVAL right after a failure to find an element's reference. I hope 
will make the code more clear to read. So there is no need to modify 
tplg_copy_data() now, checking its return value is enough.

The new patch name is [PATCH v4] topology: Return -EINVAL at once on 
failure to find a reference. Please review.

Thanks
Mengdong

On 07/22/2016 11:28 AM, Takashi Sakamoto wrote:
> On Jul 22 2016 11:39, Lin, Mengdong wrote:
>>> -----Original Message-----
>>> From: Takashi Sakamoto [mailto:o-takashi@sakamocchi.jp]
>>> Sent: Friday, July 22, 2016 10:16 AM
>>> To: mengdong.lin@linux.intel.com; alsa-devel@alsa-project.org;
>>> broonie@kernel.org
>>> Cc: tiwai@suse.de; Lin, Mengdong; Girdwood, Liam R; Nc, Shreyas
>>> Subject: Re: [PATCH v2 2/2] topology: Fix the missing referenced elem
>>> ptr
>>> when merging private data
>>>
>>> On Jul 22 2016 10:47, mengdong.lin@linux.intel.com wrote:
>>>> From: Mengdong Lin <mengdong.lin@linux.intel.com>
>>>>
>>>> tplg_copy_data() should set the valid referenced data element pointer
>>>> on success. The caller will double check this pointer for all kinds of
>>>> references, including controls and data.
>>>>
>>>> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
>>>>
>>>> diff --git a/src/topology/data.c b/src/topology/data.c index
>>>> 768fc27..e7793b2 100644
>>>> --- a/src/topology/data.c
>>>> +++ b/src/topology/data.c
>>>> @@ -1078,6 +1078,8 @@ int tplg_copy_data(snd_tplg_t *tplg, struct
>>> tplg_elem *elem,
>>>>        ref_elem->compound_elem = 1;
>>>>        memcpy(priv->data + old_priv_data_size,
>>>>               ref_elem->data->data, priv_data_size);
>>>> +
>>>> +    ref->elem = ref_elem;
>>>>        return 0;
>>>>    }
>>>
>>> Is this really OK when the found topology element has no private data?
>>> In this case, ref->elem has no assignment at return.
>>
>> No. This is my mistake. Thanks for finding this bug.
>>
>> ref->elem should still be assigned for this case. I will make it like
>> this:
>>
>> /* overlook empty private data */
>> if (!ref_elem->data || !ref_elem->data->size) {
>>         ref->elem = ref_elem;
>>           return 0;
>> }
>
> Let us split data merging code to an explicit function? Then:
>
> if (ref_elem->data != NULL && ref_elem->data->size > 0) {
>      err = merge_private_data(elem, ref_elem);
>      if (err < 0)
>          return err;
> }
> ref->elem = ref_elem;
> return 0;
>
> (Here, I expect compiler optimization to merge the new function to
> caller implicitly.)
>
>
> Regards
>
> Takashi Sakamoto
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>