[vlc-commits] [Git][videolan/vlc][master] 5 commits: cmdline: fix double-printing of obsolete core option warnings
Rémi Denis-Courmont (@Courmisch)
gitlab at videolan.org
Thu Jan 27 07:25:20 UTC 2022
Rémi Denis-Courmont pushed to branch master at VideoLAN / VLC
Commits:
c048cf7d by Lyndon Brown at 2022-01-27T06:51:46+00:00
cmdline: fix double-printing of obsolete core option warnings
Note that `b_ignore_errors` does not apply to errors only but warnings too.
It's used to disable problem reporting for the preliminary parsing stage.
For example `vlc --scale 2.0` would result in:
```
VLC media player 4.0.0-dev Otto Chriek (revision 4.0.0-dev-17920-g00a114229c)
Warning: Option --scale no longer exists.
Warning: Option --scale no longer exists.
```
- - - - -
7f369576 by Lyndon Brown at 2022-01-27T06:51:46+00:00
core: improve some init stage comments
The old comments against the actions of (1) loading of saved-settings, and
(2) the second-stage of commandline argument processing, made it sound like
firstly saved settings were loaded, then that state was overwritten by
state from use of CLI options, and thus the order of these two actions
needed to be in this particular order. In truth the two sets of config
state are stored separately and their order makes no difference to CLI args
taking priority in runtime behaviour.
The comment against use of `module_InitBank()` was incorrect since we don't
output core-only help early.
The comment against the first commandline argument processing stage (the
first use of `config_LoadCmdLine()`) was misleading, since in fact it does
not just "get" use of "options that affect module loading", but rather it
runs the processing code in a mode whereby error/warning output is disabled
and with it only having knowledge of the core option set, which results in
it broadly "getting" use of any and all core options (though this clumsy
early processing is imperfect).
- - - - -
cf63125f by Lyndon Brown at 2022-01-27T06:51:46+00:00
core: move saved settings handling to after full cmdline arg processing
Switching the order here is necessary to prepare for reworking the
preliminary commandline argument parsing, since the intention is for that
to now explicitly just check for the options that affect plugin loading,
and thus use of core options regarding the saved settings file will not
have been captured yet at the point this was being done.
Additionally, it is simply more logical to do it after we've *completely*
parsed the cmdline args. Loading the saved settings will have been a
complete waste of effort should an error condition occur in cmdline
parsing.
Note that this in no way affects how CLI option use is prioritised over
saved settings for runtime behaviour.
- - - - -
f9b347e7 by Lyndon Brown at 2022-01-27T06:51:46+00:00
cmdline: re-implement the preliminary commandline argument processing
Processing of commandline arguments is done in two passes. The first is
done with only knowledge of the core option set, to pick up any use of a
few options that affect plugin data loading (`--plugins-cache`,
`--plugins-scan` and `--reset-plugins-cache`). After that is done then we
can load the plugin data, thus gaining knowledge of the full set of
available options, putting us in position to do the second pass, which
fully and properly processes everything.
The way the first pass was implemented was to run the normal processing
function with only the core option set loaded and with error/warning
reporting disabled.
This design was not ideal for two reasons:
1. It was unnecessarily inefficient:
- It involved creating a copy of the argument set to operate upon such
that the original was left intact for the second pass.
- It meant that we processed the core option set twice in creating a
representation of options to give to getopt (this was not cached and
re-used).
- It meant that every argument was compared with a set of something like
630 options in that first pass when we're only interested in looking
out for use of 9 (3 variants per bool option). (Note that vlc binaries
typically add a few arguments to the user provided set so there
typically are some arguments being processed upon every execution).
- It meant that for every core option matched we were executing the
`var_Create()`, `var_SetFoo()` and for ints/floats also `var_Change()`
functions twice over (once per pass) to store the corresponding state.
2. It was a problematic design. For certain valid and invalid use of
arguments, the results of the two passes can be very different. The
results from the first were not inline with what you would normally
expect from getopt parsing behaviour, and created a significant
undesirable behavioural difference under certain conditions between
core options and plugin options. In general you'd ultimately end up
with a mishmash of the two results. (The results of the second merged
over the top of the first, but not necessarily fully overwriting
everything). (Please read the documentation of the new
`config_CmdLineEarlyScan()` function to better understand this aspect).
To demonstrate the correctness/consistency issue, compare the results of
`vlc --gain --help` with `vlc --sepia-intensity --help`. In the former case
`--help` is consumed as the value for `--gain`, as it should be per getopt
parsing rules (despite how illogical this may seem - read the documentation
mentioned above that talks about this). In the latter example, you see the
help output. why? Because in the second pass it got similarly eaten up, but
in the first pass the `--sepia-intensity` option was unknown and so skipped
over, letting `--help` be picked up; the two results got merged, letting
`--help` incorrectly be applied (which is technically wrong behaviour).
This commit replaces the first-pass with a new function that very simply
scans over the argument set looking out for use of just those three very
specific options. This is vastly more efficient relatively speaking, and
solves the "correct" and "consistent" parsing issue as best as we can
without dropping the need for a preliminary scan.
- - - - -
26ec9983 by Lyndon Brown at 2022-01-27T06:51:46+00:00
cmdline: re-indent
- - - - -
3 changed files:
- src/config/cmdline.c
- src/config/configuration.h
- src/libvlc.c
Changes:
=====================================
src/config/cmdline.c
=====================================
@@ -40,6 +40,94 @@
#include <assert.h>
+#ifdef HAVE_DYNAMIC_PLUGINS
+#undef config_CmdLineEarlyScan
+/**
+ * Perform early scan of arguments for a small subset of simple options.
+ *
+ * Before proper full processing can be done, which requires full knowledge of
+ * all available options, we have a need to perform a preliminary check for a
+ * few options that affect plugin loading. This is because the full option set
+ * consists of a core subset plus various plugin subsets and we thus need to
+ * load plugin data to know the plugin subsets, but there are a few core options
+ * that actually affect loading of plugin data.
+ *
+ * Note that we want to be cautious about doing too much here. Firstly simply
+ * to avoid duplicating effort done later in the proper full processing step.
+ * Additionally though because fundamentally the very notion of a preliminary
+ * scan is problematic in terms of taking "correct" (or at least consistent)
+ * action in the face of certain use or misuse of options. Consider the fact
+ * that it is standard practice with option arguments that take a value, should
+ * a value not be provided within the same argument, to just consume the next
+ * argument as the option's value, no matter whether or not the next argument
+ * might "look" like an option itself (or an "early terminator"). Such is the
+ * behaviour of getopt. (It may perhaps seem wrong to do things that way, but
+ * that's how it works; it would alternatively be wrong for getopt in general to
+ * prevent options from taking values that just happen to resemble these things;
+ * and besides, bad consumption will only occur through bad option use which is
+ * fair to expect to cause incorrect results and in many cases ideally would be
+ * caught early by validation checks on option values anyway). So, a preliminary
+ * scan, by not knowing the full option set and not following the standard
+ * parsing rules (since it can't), naturally introduces risks of "incorrect" or
+ * "inconsistent" interpretation of the set of arguments, at least wrt. the
+ * later full/proper processing, and the more that we do, the more such
+ * incorrect/inconsistent processing scenarios we introduce. Furthermore
+ * consider the additional complication of short option "sets" (which is a short
+ * option argument involving multiple characters like `-abc`). In a short option
+ * set only the last option in the set (which is not necessarily its last
+ * character) can be one that takes a value, with any characters coming after it
+ * (if any) taken as its option value (if none then the next argument is
+ * consumed instead). In a preliminary scan where we do not know the full set of
+ * available short options, we can risk misinterpreting characters in short
+ * option sets as options when they are meant to be consumed as an option value.
+ * Thus it is very problematic to properly try to deal with an option like
+ * `--help`, which comes with the short option `-h` as well as `--no-help`, here
+ * in a preliminary scan, which is unfortunate because it might make a nice
+ * optimisation if we could.
+ *
+ * Considering this discussion, it is simply best that we keep option checks
+ * here to an absolute bare minimum of just the three plugin data related ones
+ * that we have no choice but to scan for early, and we must put up with a
+ * minimal amount of possible incorrect/inconsistent interpretation as
+ * unavoidable. (Though we could add some code after the full scan to determine
+ * whether or not the wrong action took place and issue a warning if so)..
+ *
+ * @param p_this object to write command line options as variables to
+ * @param argc number of command line arguments
+ * @param argv command line arguments
+ */
+void config_CmdLineEarlyScan( vlc_object_t *p_this, int argc, const char *argv[] )
+{
+ for( int i = 0; i < argc; i++ )
+ {
+ const char *arg = argv[i];
+
+ /* Early terminator */
+ if( strcmp( arg, "--" ) == 0 )
+ break;
+
+#define check_option_variant(option, option_name, value) \
+ if( strcmp( arg, option ) == 0 ) \
+ { \
+ var_Create (p_this, option_name, VLC_VAR_BOOL); \
+ var_SetBool (p_this, option_name, value); \
+ continue; \
+ }
+#define check_option(option_name) \
+ check_option_variant("--" option_name, option_name, true) \
+ check_option_variant("--no-"option_name, option_name, false) \
+ check_option_variant("--no" option_name, option_name, false)
+
+ check_option("plugins-cache")
+ check_option("plugins-scan")
+ check_option("reset-plugins-cache")
+
+#undef check_option
+#undef check_option_variant
+ }
+}
+#endif
+
#undef config_LoadCmdLine
/**
* Parse command line for configuration options.
@@ -51,9 +139,8 @@
*
* @param p_this object to write command line options as variables to
* @param i_argc number of command line arguments
- * @param ppsz_args commandl ine arguments [IN/OUT]
- * @param pindex NULL to ignore unknown options,
- * otherwise index of the first non-option argument [OUT]
+ * @param ppsz_args command line arguments [IN/OUT]
+ * @param pindex index of the first non-option argument [OUT]
* @return 0 on success, -1 on error.
*/
int config_LoadCmdLine( vlc_object_t *p_this, int i_argc,
@@ -61,8 +148,6 @@ int config_LoadCmdLine( vlc_object_t *p_this, int i_argc,
{
int i_cmd, i_index, i_opts, i_shortopts, flag, i_verbose = 0;
struct vlc_option *p_longopts;
- const char **argv_copy = NULL;
-#define b_ignore_errors (pindex == NULL)
/* Short options */
i_shortopts = 0;
@@ -91,22 +176,6 @@ int config_LoadCmdLine( vlc_object_t *p_this, int i_argc,
return -1;
}
- /* If we are requested to ignore errors, then we must work on a copy
- * of the ppsz_argv array, otherwise getopt_long will reorder it for
- * us, ignoring the arity of the options */
- if( b_ignore_errors )
- {
- argv_copy = vlc_alloc( i_argc, sizeof(char *) );
- if( argv_copy == NULL )
- {
- free( psz_shortopts );
- free( p_longopts );
- return -1;
- }
- memcpy( argv_copy, ppsz_argv, i_argc * sizeof(char *) );
- ppsz_argv = argv_copy;
- }
-
/* Indicate that we want to know the difference between unknown option and
missing option value issues */
psz_shortopts[0] = ':';
@@ -290,56 +359,53 @@ int config_LoadCmdLine( vlc_object_t *p_this, int i_argc,
}
/* Internal error: unknown option or missing option value */
- if( !b_ignore_errors )
+ char *optlabel;
+ if ( (state.opt && asprintf(&optlabel, "%s-%c%s",
+ color ? TS_YELLOW : "", state.opt,
+ color ? TS_RESET : "") < 0)
+ || (!state.opt && asprintf(&optlabel, "%s%s%s",
+ color ? TS_YELLOW : "", ppsz_argv[state.ind-1],
+ color ? TS_RESET : "") < 0) )
{
- char *optlabel;
- if ( (state.opt && asprintf(&optlabel, "%s-%c%s",
- color ? TS_YELLOW : "", state.opt,
- color ? TS_RESET : "") < 0)
- || (!state.opt && asprintf(&optlabel, "%s%s%s",
- color ? TS_YELLOW : "", ppsz_argv[state.ind-1],
- color ? TS_RESET : "") < 0) )
- {
- /* just ignore failure - unlikely and not worth trying to handle in some way */
- optlabel = NULL;
- }
+ /* just ignore failure - unlikely and not worth trying to handle in some way */
+ optlabel = NULL;
+ }
- fprintf( stderr, _( "%sError:%s " ), color ? TS_RED_BOLD : "", color ? TS_RESET : "");
- if (i_cmd == ':')
- {
- fprintf( stderr, _( "Missing mandatory value for option %s\n" ), optlabel );
- }
- else
- {
- fprintf( stderr, _( "Unknown option `%s'\n" ), optlabel );
+ fprintf( stderr, _( "%sError:%s " ), color ? TS_RED_BOLD : "", color ? TS_RESET : "");
+ if (i_cmd == ':')
+ {
+ fprintf( stderr, _( "Missing mandatory value for option %s\n" ), optlabel );
+ }
+ else
+ {
+ fprintf( stderr, _( "Unknown option `%s'\n" ), optlabel );
- /* suggestion matching */
- if( !state.opt )
- {
- float jw_filter = 0.8, best_metric = jw_filter, metric;
- const char *best = NULL;
- const char *jw_a = ppsz_argv[state.ind-1] + 2;
- for (size_t i = 0; i < (size_t)i_opts; i++) {
- if (p_longopts[i].is_obsolete)
- continue;
- const char *jw_b = p_longopts[i].name;
- if (vlc_jaro_winkler(jw_a, jw_b, &metric) == 0) { //ignore failed malloc calls
- if (metric > best_metric || (!best && metric >= jw_filter)) {
- best = jw_b;
- best_metric = metric;
- }
+ /* suggestion matching */
+ if( !state.opt )
+ {
+ float jw_filter = 0.8, best_metric = jw_filter, metric;
+ const char *best = NULL;
+ const char *jw_a = ppsz_argv[state.ind-1] + 2;
+ for (size_t i = 0; i < (size_t)i_opts; i++) {
+ if (p_longopts[i].is_obsolete)
+ continue;
+ const char *jw_b = p_longopts[i].name;
+ if (vlc_jaro_winkler(jw_a, jw_b, &metric) == 0) { //ignore failed malloc calls
+ if (metric > best_metric || (!best && metric >= jw_filter)) {
+ best = jw_b;
+ best_metric = metric;
}
}
- if (best)
- fprintf( stderr, _( " Did you mean %s--%s%s?\n" ),
- color ? TS_GREEN : "", best, color ? TS_RESET : "" );
}
+ if (best)
+ fprintf( stderr, _( " Did you mean %s--%s%s?\n" ),
+ color ? TS_GREEN : "", best, color ? TS_RESET : "" );
}
- fprintf( stderr, _( "For more information try %s--help%s\n" ),
- color ? TS_GREEN : "", color ? TS_RESET : "" );
- free( optlabel );
- goto out;
}
+ fprintf( stderr, _( "For more information try %s--help%s\n" ),
+ color ? TS_GREEN : "", color ? TS_RESET : "" );
+ free( optlabel );
+ goto out;
}
ret = 0;
@@ -351,7 +417,5 @@ out:
free( (char *)p_longopts[i_index].name );
free( p_longopts );
free( psz_shortopts );
- free( argv_copy );
return ret;
}
-
=====================================
src/config/configuration.h
=====================================
@@ -54,6 +54,11 @@ int config_AutoSaveConfigFile( vlc_object_t * );
void config_Free(struct vlc_param *, size_t);
+#ifdef HAVE_DYNAMIC_PLUGINS
+void config_CmdLineEarlyScan( vlc_object_t *, int, const char *[] );
+#define config_CmdLineEarlyScan(a,b,c) config_CmdLineEarlyScan(VLC_OBJECT(a),b,c)
+#endif
+
int config_LoadCmdLine ( vlc_object_t *, int, const char *[], int * );
int config_LoadConfigFile( vlc_object_t * );
#define config_LoadCmdLine(a,b,c,d) config_LoadCmdLine(VLC_OBJECT(a),b,c,d)
=====================================
src/libvlc.c
=====================================
@@ -139,29 +139,38 @@ int libvlc_InternalInit( libvlc_int_t *p_libvlc, int i_argc,
/* System specific initialization code */
system_Init();
- /* Initialize the module bank and load the configuration of the
- * core module. We need to do this at this stage to be able to display
- * a short help if required by the user. (short help == core module
- * options) */
+ /*
+ * Initialize the module bank and load the core config only.
+ */
module_InitBank ();
- /* Get command line options that affect module loading. */
- if( config_LoadCmdLine( p_libvlc, i_argc, ppsz_argv, NULL ) )
- {
- module_EndBank (false);
- return VLC_EGENERIC;
- }
+ /*
+ * Perform early check for commandline arguments that affect module loading.
+ */
+#ifdef HAVE_DYNAMIC_PLUGINS
+ config_CmdLineEarlyScan( p_libvlc, i_argc, ppsz_argv );
+#endif
vlc_threads_setup (p_libvlc);
- /* Load the builtins and plugins into the module_bank.
- * We have to do it before config_Load*() because this also gets the
- * list of configuration options exported by each module and loads their
- * default values. */
+ /*
+ * Load plugin data into the module bank.
+ * We need to do this here such that option sets from plugins are added to
+ * the config system in order that full commandline argument parsing and
+ * saved settings handling can function properly.
+ */
module_LoadPlugins (p_libvlc);
/*
- * Override default configuration with config file settings
+ * Fully process command line settings.
+ * Results are stored as runtime state within `p_libvlc` object variables.
+ */
+ int vlc_optind;
+ if( config_LoadCmdLine( p_libvlc, i_argc, ppsz_argv, &vlc_optind ) )
+ goto error;
+
+ /*
+ * Load saved settings into the config system, as applicable.
*/
if( !var_InheritBool( p_libvlc, "ignore-config" ) )
{
@@ -171,13 +180,6 @@ int libvlc_InternalInit( libvlc_int_t *p_libvlc, int i_argc,
config_LoadConfigFile( p_libvlc );
}
- /*
- * Override configuration with command line settings
- */
- int vlc_optind;
- if( config_LoadCmdLine( p_libvlc, i_argc, ppsz_argv, &vlc_optind ) )
- goto error;
-
vlc_LogInit(p_libvlc);
vlc_tracer_Init(p_libvlc);
@@ -191,6 +193,9 @@ int libvlc_InternalInit( libvlc_int_t *p_libvlc, int i_argc,
/*xgettext: Translate "C" to the language code: "fr", "en_GB", "nl", "ru"... */
msg_Dbg( p_libvlc, "translation test: code is \"%s\"", _("C") );
+ /*
+ * Handle info requests such as for help or version text.
+ */
if (config_PrintHelp (VLC_OBJECT(p_libvlc)))
{
libvlc_InternalCleanup (p_libvlc);
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/def4dea4dc7470aa6cfc96a4ca99975f041d4954...26ec998368eec443df5027530e3507717ae0ef42
--
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/def4dea4dc7470aa6cfc96a4ca99975f041d4954...26ec998368eec443df5027530e3507717ae0ef42
You're receiving this email because of your account on code.videolan.org.
More information about the vlc-commits
mailing list