[pulseaudio-discuss] Revisiting raop2 patches

[Hajime Fujita]

Anton Lundin wrote:
> On 30 October, 2016 - Hajime Fujita wrote:
>>
>>> On Oct 26, 2016, at 9:58 PM, Hajime Fujita <crisp.fujita at nifty.com> wrote:
>>>
>>>> On Oct 25, 2016, at 3:47 PM, Anton Lundin <glance at acc.umu.se> wrote:
>>>
>>>> Also, value is set to NULL in some parsing blocks and not in others.
>>>> Some blocks strdup(value), and let avahi_free(value) free it, others
>>>> "steal" value and set value = NULL. I'd feel more comfortable with all
>>>> the code paths strdup'ing them selfs. Even, why not use pa_xstrdup ?
>>>
>>> Will take a look.
>>
>> So I took a look at this.
>>
>> To me, this code makes some sense.
>> Strdup is used when the raop module generates a new string based on information in a value given by Avahi.
>> Value “stealing” happens when the raop module uses an Avahi string as-is. In this case strdup’ing is essentially a waste of memory and allocation/free cost. (Nobody probably cares about the cost here though)
>>
>> “strdup” should be “pa_xstrdup”, I agree.
>>
> 
> Yea, its some added cost in the discovery process, the thing that stands
> out to me is that the code paths are different. Different means
> complexity and complexity menas potential for bugs.
> 
> Yea, its a simple case, but I don't know the allocation rules for
> avahi-strings. Its better to handle all of that logic right there and
> then, and let PA own the strings from there and onwards.
> 
> It's also the case of keeping track of whats avahi-memory and whats
> pa-memory. I'd guess pa_xfree() and avahi_free() are compatible wrappers
> ontop of free(), but who knows. Some day some crazy person might change
> one of them to be a completely different beast, and things will start to
> behave strangely.
> 
> In such a code path as this one, I'd vote for clarity and robustness
> every day of the week.
> 
> 
> One can clearly see that module-raop-discover.c inherited alot of code
> from module-zeroconf-discover.c, and this odity is there to. The same
> goes for ex. that the hashmap that keeps track of all loaded raop
> modules are called tunnels.

Okay, I understand your concern really is a valid point. I don't have strong preference for either side, so simply follow your suggestion.


Thanks,
Hajime
 
> //Anton - who clearly hears the bikeshedding warning bells going off
> 
>