[MPlayer-dev-eng] CONF_TYPE_STRING issue

[Ingo Brückl]

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?

Ingo