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

[Tanu Kaskinen]

On Thu, 2013-05-30 at 12:46 +0200, David Henningsson wrote:
> On 05/30/2013 10:26 AM, Tanu Kaskinen wrote:
> > On Thu, 2013-05-30 at 01:45 +0200, David Henningsson wrote:
> >> 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?
> >
> > I intend to use the sink/source description as the node description too
> > (for nodes that are backed by a sink or source, that is - if ports
> > exist, they are used as the node backend instead, but not all sinks and
> > sources are associated with a port).
> >
> >>    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?
> >
> > How could there possibly be no relevant information to add? Surely every
> > sink can have a sane human-readable description?
> >
> > pavucontrol needs a description anyway, because it uses sinks and
> > sources as UI elements. For this reason I wouldn't be surprised if the
> > situation already was such that every sink and source sets
> > PA_PROP_DEVICE_DESCRIPTION.
> >
> >>    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).
> >
> > Uh, it seems that you think that sinks have a separate description in
> > addition to PA_PROP_DEVICE_DESCRIPTION. If they did, I'd have no
> > problem.
> >
> 
> Ah, it seems you are right. The PA_PROP_DEVICE_DESCRIPTION is being used 
> as a de-facto description of the sink (look e g in protocol-native where 
> a sink description is filled in).
> 
> It's probably too risky w r t existing clients to remove it from the 
> proplist.

Definitely.

> And I don't like storing the same information twice, so would 
> it be okay to have a pa_sink_get_description function?

I guess that would be fine.

> Then I don't mind making it mandatory, assuming you go through all 
> existing sink/source implementations and verify that it is filled in 
> correctly.

I'll do that.

-- 
Tanu