[MPlayer-dev-eng] CONF_TYPE_STRING issue

[Ingo Brückl]

I wrote on Fri, 06 May 2011 12:00:30 +0200:

> Reimar Döffinger wrote on Thu, 5 May 2011 18:28:03 +0200:

>>> Wouldn't this give direct access to the slot data, meaning that changing
>>> the variable (for what reason ever) disables later restoring it from the
>>> initial config parsed value?

>> By what I can tell from the code it's not supposed to restore to the
>> config parsed value.

> If so, ok, although the slot data (copy) seems useless then.

>> However I realized it didn't quite do what I intended, does attached
>> patch look better to you?

> I don't can say much about @@ -342,10 +343,7 @@ in m_config.c, but that's
> only due to my lack of complete understanding of the config code.

> The rest looks good.

>> It should avoid the issue you pointed out: setting the variable to
>> a non-malloced value. It is not necessary to support it, but if we can...

> Well, that allows the non-malloced value *after* parsing, but now disallows
> a malloced value. (I'm not sure which is the better. Please remember that
> there wouldn't be a problem at all with a non-malloced value *default*
> value, i.e. set *before* parsing.)

> Although I think that

>   free(option);
>   option = strdup("new_option");

> is a much more allowable thing to do after parsing (more allowable at least
> than

>   option = "new_option";

> after parsing), I admit that the former now only will end up where we
> started from - allocated (config) memory, not getting freed (whereas my
> approach crashes at the latter).

> On the other hand, if you have to change an option after parsing, because
> you have - for example - a config dialog and an input box where you give
> the new value, it's most certainly the former code you are using.

> So now:

>   option = "new_option";                         // is ok
>   free(option); option = strdup("new_option");   // is not ok...

>   // ...use this instead
>   m_config_set_option(conf, "option_name", "new_option");

> Not really satisfying.

> All in all, freeing the variable and considering a non-malloced value
> *after* parsing a programming error (because m_config_set_option() should
> be used to change options) would be much simpler in both understanding and
> new code size, wouldn't it?

Ping.

Still an open issue.

Ingo