2017-04-12 20:34 GMT+09:00 Georg Chini <georg at chini.tk>:
> The first two patches of the series look good to me (apart from
> spelling/grammar).
> A few comments to this patch below.
Ok, I did submit patch v3 again according to your review.
>> @@ -140,6 +144,26 @@ static const char* should_filter(pa_object *o, bool
>> is_sink_input) {
>> return NULL;
>> }
>> +static const char* should_filter_parameters(pa_object *o, const char
>> *want, bool is_sink_input) {
>
>
> I don't like the name of the function. Can't you call it
> get_filter_parameters() and maybe also
> rename should_filter() to get_filter_name()? I would expect a function
> starting with "should"
> to return a boolean.
You are right. Fixed,
>> + const char *parameters;
>> + char *prop_parameters;
>> + pa_proplist *pl;
>> +
>> + if (is_sink_input)
>> + pl = PA_SINK_INPUT(o)->proplist;
>> + else
>> + pl = PA_SOURCE_OUTPUT(o)->proplist;
>> +
>> + prop_parameters = pa_sprintf_malloc(PA_PROP_FILTER_APPLY_PARAMETERS,
>> want);
>> + if ((parameters = pa_proplist_gets(pl, prop_parameters)) &&
>> !pa_streq(parameters, "")) {
>
>
> Why do you check for the empty string here? If this is not needed you could
> leave out
> the if () condition completely and just return parameters. As far as I can
> tell an empty
> string would not do any harm.
Fixed as you suggested.
>> @@ -517,7 +547,11 @@ static pa_hook_result_t process(struct userdata *u,
>> pa_object *o, bool is_put_or
>> done:
>> pa_xfree(module_name);
>> - pa_xfree(fltr);
>> + if (fltr) {
>> + pa_xfree(fltr->name);
>> + pa_xfree(fltr->parameters);
>> + pa_xfree(fltr);
>> + }
>>
>
>
> This is (partly) redundant with your "Fix memory leak" patch. As already
> said, I would use
> filter_free() here after modification.
Ok, it fixed,
Thanks for reviewing. :-)
Regards,
KimJeongYeon
