[pulseaudio-discuss] [PATCH 1/2] module-device-manager: Fix description restore

[David Henningsson]

On 05/29/2013 04:57 PM, Tanu Kaskinen wrote:
>
> On Wed, 2013-05-22 at 14:45 +0300, Tanu Kaskinen wrote:
>> On Wed, 2013-05-22 at 13:33 +0200, David Henningsson wrote:
>>> e->description is a pointer, not a fixed char array. Hence it
>>> makes no sense to use strncmp.
>>>
>>> This fixes a compiler warning when compiling under Ubuntu.
>>>
>>> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
>>> ---
>>>   src/modules/module-device-manager.c |    4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/modules/module-device-manager.c b/src/modules/module-device-manager.c
>>> index 207870d..390046f 100644
>>> --- a/src/modules/module-device-manager.c
>>> +++ b/src/modules/module-device-manager.c
>>> @@ -889,7 +889,7 @@ static pa_hook_result_t sink_new_hook_callback(pa_core *c, pa_sink_new_data *new
>>>       name = pa_sprintf_malloc("sink:%s", new_data->name);
>>>
>>>       if ((e = entry_read(u, name))) {
>>> -        if (e->user_set_description && strncmp(e->description, pa_proplist_gets(new_data->proplist, PA_PROP_DEVICE_DESCRIPTION), sizeof(e->description)) != 0) {
>>> +        if (e->user_set_description && strcmp(e->description, pa_proplist_gets(new_data->proplist, PA_PROP_DEVICE_DESCRIPTION)) != 0) {
>>
>> pa_proplist_gets() can return NULL, and if it does that, strcmp() will
>> crash.
>
> Actually, I think we should make sure that all sink implementations set
> PA_PROP_DEVICE_DESCRIPTION in new_data->proplist. Or even better, we
> could have a separate description field in the new_data struct. I'm
> dealing with sink descriptions myself currently, and I find it annoying
> having to access them through proplists, and if I can't assume that the
> property has been set, it's even more annoying.
>

Hmm, can you explain why
  1) You need a PA_PROP_DEVICE_DESCRIPTION for sinks (and sources, I 
assume) in the first place?
  2) Why you need it so much that we should artificially create one in 
all sink implementations even if there is no relevant information to add?
  3) Why you can't write a helper function/macro if you find it annoying 
to access the proplist in several places?

My biggest worry is 2) here - we shouldn't create more information just 
for the sake of it. I don't know what you want it for, but maybe a fall 
back would be better - e g, using the sink description if there is no 
device description property. This would be handled by the helper 
function suggested in 3).


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic