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

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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