r34257 - in trunk: cfg-common.h cfg-mplayer.h m_option.c m_option.h
Author: reimar Date: Tue Oct 25 22:18:35 2011 New Revision: 34257 Log: Sanitize include behaviour. The normal func_param argument type will iterate over all previous values each time a new value is assigned. This leads e.g. to a complete mess and non-working recursion limiting when creating a config file that includes itself. Seem to also fix bug #1994. Modified: trunk/cfg-common.h trunk/cfg-mplayer.h trunk/m_option.c trunk/m_option.h Modified: trunk/cfg-common.h ============================================================================== --- trunk/cfg-common.h Tue Oct 25 21:22:10 2011 (r34256) +++ trunk/cfg-common.h Tue Oct 25 22:18:35 2011 (r34257) @@ -305,7 +305,7 @@ const m_option_t common_opts[] = { #ifdef CONFIG_ICONV {"msgcharset", &mp_msg_charset, CONF_TYPE_STRING, CONF_GLOBAL, 0, 0, NULL}, #endif - {"include", cfg_include, CONF_TYPE_FUNC_PARAM, CONF_NOSAVE, 0, 0, NULL}, + {"include", cfg_include, CONF_TYPE_FUNC_PARAM_IMMEDIATE, CONF_NOSAVE, 0, 0, NULL}, #ifdef CONFIG_PRIORITY {"priority", &proc_priority, CONF_TYPE_STRING, 0, 0, 0, NULL}, #endif Modified: trunk/cfg-mplayer.h ============================================================================== --- trunk/cfg-mplayer.h Tue Oct 25 21:22:10 2011 (r34256) +++ trunk/cfg-mplayer.h Tue Oct 25 22:18:35 2011 (r34257) @@ -301,7 +301,7 @@ const m_option_t mplayer_opts[]={ {"noenqueue", &enqueue, CONF_TYPE_FLAG, 0, 1, 0, NULL}, {"guiwid", "-guiwid has been removed, use -gui-wid instead.\n", CONF_TYPE_PRINT, 0, 0, 0, NULL}, {"gui-wid", &guiWinID, CONF_TYPE_INT, 0, 0, 0, NULL}, - {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM, CONF_NOSAVE, 0, 0, NULL}, + {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM_IMMEDIATE, CONF_NOSAVE, 0, 0, NULL}, #endif {"noloop", &mpctx_s.loop_times, CONF_TYPE_FLAG, 0, 0, -1, NULL}, Modified: trunk/m_option.c ============================================================================== --- trunk/m_option.c Tue Oct 25 21:22:10 2011 (r34256) +++ trunk/m_option.c Tue Oct 25 22:18:35 2011 (r34257) @@ -715,6 +715,25 @@ const m_option_type_t m_option_type_stri /////////////////// Func based options +static int parse_call_func(const m_option_t* opt,const char *name, const char *param, void* dst, int src) { + ((m_opt_func_param_t) opt->p)(opt,param); +} + +// special variant, will not have a history/be able to +// be used as per-file option etc. +const m_option_type_t m_option_type_func_param_immediate = { + "Func param once", + "", + 0, + M_OPT_TYPE_INDIRECT, + parse_call_func, + NULL, + NULL, // Nothing to do on save + NULL, + NULL, + NULL +}; + // A chained list to save the various calls for func_param and func_full typedef struct m_func_save m_func_save_t; struct m_func_save { Modified: trunk/m_option.h ============================================================================== --- trunk/m_option.h Tue Oct 25 21:22:10 2011 (r34256) +++ trunk/m_option.h Tue Oct 25 22:18:35 2011 (r34257) @@ -62,6 +62,7 @@ extern const m_option_type_t m_option_ty // Func-based types extern const m_option_type_t m_option_type_func_full; extern const m_option_type_t m_option_type_func_param; +extern const m_option_type_t m_option_type_func_param_immediate; extern const m_option_type_t m_option_type_func; /// Callback used to reset func options. @@ -177,6 +178,7 @@ extern const m_obj_params_t m_span_param #define CONF_TYPE_STRING (&m_option_type_string) #define CONF_TYPE_FUNC (&m_option_type_func) #define CONF_TYPE_FUNC_PARAM (&m_option_type_func_param) +#define CONF_TYPE_FUNC_PARAM_IMMEDIATE (&m_option_type_func_param_immediate) #define CONF_TYPE_PRINT (&m_option_type_print) #define CONF_TYPE_PRINT_INDIRECT (&m_option_type_print_indirect) #define CONF_TYPE_PRINT_FUNC (&m_option_type_print_func)
reimar wrote on Tue, 25 Oct 2011 22:18:35 +0200 (CEST):
Author: reimar Date: Tue Oct 25 22:18:35 2011 New Revision: 34257
Log: Sanitize include behaviour.
The normal func_param argument type will iterate over all previous values each time a new value is assigned. This leads e.g. to a complete mess and non-working recursion limiting when creating a config file that includes itself. Seem to also fix bug #1994.
Modified: trunk/cfg-common.h trunk/cfg-mplayer.h trunk/m_option.c trunk/m_option.h
Modified: trunk/cfg-mplayer.h ============================================================================== +++ trunk/cfg-mplayer.h Tue Oct 25 22:18:35 2011 (r34257) @@ -301,7 +301,7 @@ const m_option_t mplayer_opts[]={ {"noenqueue", &enqueue, CONF_TYPE_FLAG, 0, 1, 0, NULL}, {"guiwid", "-guiwid has been removed, use -gui-wid instead.\n", CONF_TYPE_PRINT, 0, 0, 0, NULL}, {"gui-wid", &guiWinID, CONF_TYPE_INT, 0, 0, 0, NULL}, - {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM, CONF_NOSAVE, 0, 0, NULL}, + {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM_IMMEDIATE, CONF_NOSAVE, 0, 0, NULL},
This segfaults the GUI. If I understand CONF_TYPE_FUNC_PARAM_IMMEDIATE correctly, this isn't the desired behaviour either. Option gui-include is meant to override the gui.conf settings. In order to do this, MPlayer must have called cfg_read() where gui.conf is read and m_config_t *gui_conf is set. CONF_TYPE_FUNC_PARAM_IMMEDIATE seems to act prior to this, thus gui_conf still is NULL. As the GUI's config files don't allow includes anyway, it seems save to stay with CONF_TYPE_FUNC_PARAM. Ingo
On Wed, Oct 26, 2011 at 07:31:15PM +0200, Ingo Brückl wrote:
reimar wrote on Tue, 25 Oct 2011 22:18:35 +0200 (CEST):
Author: reimar Date: Tue Oct 25 22:18:35 2011 New Revision: 34257
Log: Sanitize include behaviour.
The normal func_param argument type will iterate over all previous values each time a new value is assigned. This leads e.g. to a complete mess and non-working recursion limiting when creating a config file that includes itself. Seem to also fix bug #1994.
Modified: trunk/cfg-common.h trunk/cfg-mplayer.h trunk/m_option.c trunk/m_option.h
Modified: trunk/cfg-mplayer.h ============================================================================== +++ trunk/cfg-mplayer.h Tue Oct 25 22:18:35 2011 (r34257) @@ -301,7 +301,7 @@ const m_option_t mplayer_opts[]={ {"noenqueue", &enqueue, CONF_TYPE_FLAG, 0, 1, 0, NULL}, {"guiwid", "-guiwid has been removed, use -gui-wid instead.\n", CONF_TYPE_PRINT, 0, 0, 0, NULL}, {"gui-wid", &guiWinID, CONF_TYPE_INT, 0, 0, 0, NULL}, - {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM, CONF_NOSAVE, 0, 0, NULL}, + {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM_IMMEDIATE, CONF_NOSAVE, 0, 0, NULL},
This segfaults the GUI.
If I understand CONF_TYPE_FUNC_PARAM_IMMEDIATE correctly, this isn't the desired behaviour either.
Option gui-include is meant to override the gui.conf settings. In order to do this, MPlayer must have called cfg_read() where gui.conf is read and m_config_t *gui_conf is set. CONF_TYPE_FUNC_PARAM_IMMEDIATE seems to act prior to this, thus gui_conf still is NULL.
As the GUI's config files don't allow includes anyway, it seems save to stay with CONF_TYPE_FUNC_PARAM.
Feel free to revert, crashes are definitely no good. However you will end up with some really strange behaviour if someone ever uses multiple gui_include (recursion is just the case where it is most obvious). Hm, if it's not supposed to work properly with multiple includes etc. anyway, why going this complicated way? You could just make it a string list or whatever and just read them all in the cfg_read function. And rather not call it include, when it doesn't really behave like that.
Reimar Döffinger wrote on Wed, 26 Oct 2011 20:22:53 +0200:
On Wed, Oct 26, 2011 at 07:31:15PM +0200, Ingo Brückl wrote:
reimar wrote on Tue, 25 Oct 2011 22:18:35 +0200 (CEST):
Author: reimar Date: Tue Oct 25 22:18:35 2011 New Revision: 34257
Log: Sanitize include behaviour.
The normal func_param argument type will iterate over all previous values each time a new value is assigned. This leads e.g. to a complete mess and non-working recursion limiting when creating a config file that includes itself. Seem to also fix bug #1994.
Modified: trunk/cfg-common.h trunk/cfg-mplayer.h trunk/m_option.c trunk/m_option.h
Modified: trunk/cfg-mplayer.h ========================================================================= ===== +++ trunk/cfg-mplayer.h Tue Oct 25 22:18:35 2011 (r34257) @@ -301,7 +301,7 @@ const m_option_t mplayer_opts[]={ {"noenqueue", &enqueue, CONF_TYPE_FLAG, 0, 1, 0, NULL}, {"guiwid", "-guiwid has been removed, use -gui-wid instead.\n", CONF_TYPE_PRINT, 0, 0, 0, NULL}, {"gui-wid", &guiWinID, CONF_TYPE_INT, 0, 0, 0, NULL}, - {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM, CONF_NOSAVE, 0, 0, NULL}, + {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM_IMMEDIATE, CONF_NOSAVE, 0, 0, NULL},
This segfaults the GUI.
If I understand CONF_TYPE_FUNC_PARAM_IMMEDIATE correctly, this isn't the desired behaviour either.
Option gui-include is meant to override the gui.conf settings. In order to do this, MPlayer must have called cfg_read() where gui.conf is read and m_config_t *gui_conf is set. CONF_TYPE_FUNC_PARAM_IMMEDIATE seems to act prior to this, thus gui_conf still is NULL.
As the GUI's config files don't allow includes anyway, it seems save to stay with CONF_TYPE_FUNC_PARAM.
However you will end up with some really strange behaviour if someone ever uses multiple gui_include (recursion is just the case where it is most obvious).
I was just doing some tests, but could not figure out any problems. Every gui-include seems to override properly what the previous gui-include may have set. Maybe you could give me a hint what to look or check for?
Hm, if it's not supposed to work properly with multiple includes etc. anyway, why going this complicated way?
Well, it seems it does.
And rather not call it include, when it doesn't really behave like that.
I does behave like include, but limited to the gui options which are a subset of the MPlayer options (unfortunately currently with different names) and a few GUI specific ones (unfortunately currently not properly using a common gui namespace). I don't think that a different option name would make sense, the gui prefix already should indicate that it is gui-ish. Ingo
On Wed, Oct 26, 2011 at 10:07:45PM +0200, Ingo Brückl wrote:
Reimar Döffinger wrote on Wed, 26 Oct 2011 20:22:53 +0200:
On Wed, Oct 26, 2011 at 07:31:15PM +0200, Ingo Brückl wrote:
reimar wrote on Tue, 25 Oct 2011 22:18:35 +0200 (CEST):
Author: reimar Date: Tue Oct 25 22:18:35 2011 New Revision: 34257
Log: Sanitize include behaviour.
The normal func_param argument type will iterate over all previous values each time a new value is assigned. This leads e.g. to a complete mess and non-working recursion limiting when creating a config file that includes itself. Seem to also fix bug #1994.
Modified: trunk/cfg-common.h trunk/cfg-mplayer.h trunk/m_option.c trunk/m_option.h
Modified: trunk/cfg-mplayer.h ========================================================================= ===== +++ trunk/cfg-mplayer.h Tue Oct 25 22:18:35 2011 (r34257) @@ -301,7 +301,7 @@ const m_option_t mplayer_opts[]={ {"noenqueue", &enqueue, CONF_TYPE_FLAG, 0, 1, 0, NULL}, {"guiwid", "-guiwid has been removed, use -gui-wid instead.\n", CONF_TYPE_PRINT, 0, 0, 0, NULL}, {"gui-wid", &guiWinID, CONF_TYPE_INT, 0, 0, 0, NULL}, - {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM, CONF_NOSAVE, 0, 0, NULL}, + {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM_IMMEDIATE, CONF_NOSAVE, 0, 0, NULL},
This segfaults the GUI.
If I understand CONF_TYPE_FUNC_PARAM_IMMEDIATE correctly, this isn't the desired behaviour either.
Option gui-include is meant to override the gui.conf settings. In order to do this, MPlayer must have called cfg_read() where gui.conf is read and m_config_t *gui_conf is set. CONF_TYPE_FUNC_PARAM_IMMEDIATE seems to act prior to this, thus gui_conf still is NULL.
As the GUI's config files don't allow includes anyway, it seems save to stay with CONF_TYPE_FUNC_PARAM.
However you will end up with some really strange behaviour if someone ever uses multiple gui_include (recursion is just the case where it is most obvious).
I was just doing some tests, but could not figure out any problems. Every gui-include seems to override properly what the previous gui-include may have set. Maybe you could give me a hint what to look or check for?
Profiles and thus probably also per-protocol or extension options probably will not be working properly. No idea if they are supposed to anyway though.
And rather not call it include, when it doesn't really behave like that.
I does behave like include, but limited to the gui options which are a subset of the MPlayer options
No, it doesn't really. As said, for example a second "include" will cause all previous includes to be re-evaluated. Especially with section this has all kinds of funny and unexpected effects. You probably should be able to also see it if you do something like gui-include=cache.conf # configures cache nocache=yes gui-include=empty.conf should result in cache being enabled at the end. Which probably can't really be considered wrong with the gui-include one though... With include this should now be working correctly.
Reimar Döffinger wrote on Fri, 28 Oct 2011 00:24:33 +0200:
On Wed, Oct 26, 2011 at 10:07:45PM +0200, Ingo Brückl wrote:
Reimar Döffinger wrote on Wed, 26 Oct 2011 20:22:53 +0200:
On Wed, Oct 26, 2011 at 07:31:15PM +0200, Ingo Brückl wrote:
reimar wrote on Tue, 25 Oct 2011 22:18:35 +0200 (CEST):
Author: reimar Date: Tue Oct 25 22:18:35 2011 New Revision: 34257
Log: Sanitize include behaviour.
The normal func_param argument type will iterate over all previous values each time a new value is assigned. This leads e.g. to a complete mess and non-working recursion limiting when creating a config file that includes itself. Seem to also fix bug #1994.
Modified: trunk/cfg-common.h trunk/cfg-mplayer.h trunk/m_option.c trunk/m_option.h
Modified: trunk/cfg-mplayer.h ========================================================================= ===== +++ trunk/cfg-mplayer.h Tue Oct 25 22:18:35 2011 (r34257) @@ -301,7 +301,7 @@ const m_option_t mplayer_opts[]={ {"noenqueue", &enqueue, CONF_TYPE_FLAG, 0, 1, 0, NULL}, {"guiwid", "-guiwid has been removed, use -gui-wid instead.\n", CONF_TYPE_PRINT, 0, 0, 0, NULL}, {"gui-wid", &guiWinID, CONF_TYPE_INT, 0, 0, 0, NULL}, - {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM, CONF_NOSAVE, 0, 0, NULL}, + {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM_IMMEDIATE, CONF_NOSAVE, 0, 0, NULL},
This segfaults the GUI.
If I understand CONF_TYPE_FUNC_PARAM_IMMEDIATE correctly, this isn't the desired behaviour either.
Option gui-include is meant to override the gui.conf settings. In order to do this, MPlayer must have called cfg_read() where gui.conf is read and m_config_t *gui_conf is set. CONF_TYPE_FUNC_PARAM_IMMEDIATE seems to act prior to this, thus gui_conf still is NULL.
As the GUI's config files don't allow includes anyway, it seems save to stay with CONF_TYPE_FUNC_PARAM.
However you will end up with some really strange behaviour if someone ever uses multiple gui_include (recursion is just the case where it is most obvious).
I was just doing some tests, but could not figure out any problems. Every gui-include seems to override properly what the previous gui-include may have set. Maybe you could give me a hint what to look or check for?
Profiles and thus probably also per-protocol or extension options probably will not be working properly. No idea if they are supposed to anyway though.
At the moment I don't see a practical use in per-any gui configs.
And rather not call it include, when it doesn't really behave like that.
I does behave like include, but limited to the gui options which are a subset of the MPlayer options
No, it doesn't really. As said, for example a second "include" will cause all previous includes to be re-evaluated. Especially with section this has all kinds of funny and unexpected effects. You probably should be able to also see it if you do something like gui-include=cache.conf # configures cache nocache=yes gui-include=empty.conf
should result in cache being enabled at the end.
This actually segfaults for the very same reason (MPlayer didn't have called cfg_read() before). I suggest the attached patch. Ingo
On Tue, Dec 06, 2011 at 06:52:14PM +0100, Ingo Brückl wrote:
This actually segfaults for the very same reason (MPlayer didn't have called cfg_read() before). I suggest the attached patch.
Ingo
Index: cfg-mplayer.h =================================================================== --- cfg-mplayer.h (revision 34368) +++ cfg-mplayer.h (working copy) @@ -301,7 +301,7 @@ {"noenqueue", &enqueue, CONF_TYPE_FLAG, 0, 1, 0, NULL}, {"guiwid", "-guiwid has been removed, use -gui-wid instead.\n", CONF_TYPE_PRINT, 0, 0, 0, NULL}, {"gui-wid", &guiWinID, CONF_TYPE_INT, 0, 0, 0, NULL}, - {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM, CONF_NOSAVE, 0, 0, NULL}, + {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM, CONF_NOCFG|CONF_NOSAVE, 0, 0, NULL}, #endif
{"noloop", &mpctx_s.loop_times, CONF_TYPE_FLAG, 0, 0, -1, NULL},
Sorry for not replying. Yes, applying this is obviously a good idea. Of course it wouldn't hurt to make it work nicely (also e.g. allowing to use it in the main config file to specify settings that should only be used when then gui is enabled), but that might be overkill...
Reimar Döffinger wrote on Sun, 11 Dec 2011 16:18:20 +0100:
On Tue, Dec 06, 2011 at 06:52:14PM +0100, Ingo Brückl wrote:
This actually segfaults for the very same reason (MPlayer didn't have called cfg_read() before). I suggest the attached patch.
Ingo
Index: cfg-mplayer.h =================================================================== --- cfg-mplayer.h (revision 34368) +++ cfg-mplayer.h (working copy) @@ -301,7 +301,7 @@ {"noenqueue", &enqueue, CONF_TYPE_FLAG, 0, 1, 0, NULL}, {"guiwid", "-guiwid has been removed, use -gui-wid instead.\n", CONF_TYPE_PRINT, 0, 0, 0, NULL}, {"gui-wid", &guiWinID, CONF_TYPE_INT, 0, 0, 0, NULL}, - {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM, CONF_NOSAVE, 0, 0, NULL}, + {"gui-include", cfg_gui_include, CONF_TYPE_FUNC_PARAM, CONF_NOCFG|CONF_NOSAVE, 0, 0, NULL}, #endif
{"noloop", &mpctx_s.loop_times, CONF_TYPE_FLAG, 0, 0, -1, NULL},
Sorry for not replying. Yes, applying this is obviously a good idea. Of course it wouldn't hurt to make it work nicely (also e.g. allowing to use it in the main config file to specify settings that should only be used when then gui is enabled), but that might be overkill...
I'll keep that in mind. Forcing CONF_NOCFG doesn't have to be the final solution. Ingo
participants (3)
-
Ingo Brückl -
reimar -
Reimar Döffinger