Hi, Kristofer!
On May 04, Kristofer Pettersson wrote:
>
> 2877 Kristofer Pettersson 2009-05-04
> Bug#19027 MySQL 5.0 starts even with Fatal InnoDB errors
Good.
I'm fine with hoe you removed 'enabled'.
But I think I've found a few bugs, see below - there's a memory
leak, incorrect default behavior, unjustified behavior change.
And a couple of smaller issues.
> === modified file 'sql/sql_plugin.cc'
> --- a/sql/sql_plugin.cc 2009-03-16 10:37:13 +0000
> +++ b/sql/sql_plugin.cc 2009-05-04 08:48:06 +0000
> @@ -29,6 +29,18 @@
>
> extern struct st_mysql_plugin *mysqld_builtins[];
>
> +/**
> + @note The order of the enumeration is critical.
> + @see construct_options
> +*/
> +static const char *global_plugin_typelib_names[]=
> + { "OFF", "ON", "FORCE", NULL };
> +enum plugin_load_policy_t {PLUGIN_OFF, PLUGIN_ON, PLUGIN_FORCE};
> +TYPELIB global_plugin_typelib= { array_elements(global_plugin_typelib_names)-1,
this should be static too (it's not needed outside of sql_plugin.cc)
> + "", global_plugin_typelib_names,
> + NULL };
> +
> +
> char *opt_plugin_load= NULL;
> char *opt_plugin_dir_ptr;
> char opt_plugin_dir[FN_REFLEN];
> @@ -2889,21 +2901,39 @@ my_bool get_one_plugin_option(int optid
> }
>
>
> +/**
> + Creates a set of my_option objects associated with a specified plugin-
> + handle.
> +
> + @param mem_root Memory allocator to be used.
> + @param tmp A pointer to a plugin handle
> + @param[out] options A pointer to a pre-allocated static array
> +
> + The set is stored in the pre-allocated static array supplied to the function.
> + The size of the array is calculated as (number_of_plugin_varaibles*2+3). The
> + reason is that each option can have a prefix '--plugin-' in addtion to the
> + shorter form '--<plugin-name>'. There is also space allocated for
> + terminating NULL pointers.
> +
> + @return
> + @retval -1 An error occurred
> + @retval 0 Success
> +*/
> +
> static int construct_options(MEM_ROOT *mem_root, struct st_plugin_int *tmp,
> - my_option *options, my_bool **enabled,
> - bool can_disable)
> + my_option *options)
> {
> const char *plugin_name= tmp->plugin->name;
> uint namelen= strlen(plugin_name), optnamelen;
> - uint buffer_length= namelen * 4 + (can_disable ? 75 : 10);
> - char *name= (char*) alloc_root(mem_root, buffer_length) + 1;
> + char *name= (char*) alloc_root(mem_root, namelen*2 + 10);
> + const int comment_length= 80+(namelen*2+10)+strlen(plugin_name);
you have strlen(plugin_name) in namelen, no need to recalculate it
> + char *comment= (char *) alloc_root(mem_root, comment_length);
> +
> char *optname, *p;
> int index= 0, offset= 0;
> st_mysql_sys_var *opt, **plugin_option;
> st_bookmark *v;
> DBUG_ENTER("construct_options");
>
> /* support --skip-plugin-foo syntax */
> memcpy(name, plugin_name, namelen + 1);
> @@ -2915,33 +2945,30 @@ static int construct_options(MEM_ROOT *m
> if (*p == '_')
> *p= '-';
>
> - if (can_disable)
> - {
> - strxmov(name + namelen*2 + 10, "Enable ", plugin_name, " plugin. "
> - "Disable with --skip-", name," (will save memory).", NullS);
> - /*
> - Now we have namelen * 2 + 10 (one char unused) + 7 + namelen + 9 +
> - 20 + namelen + 20 + 1 == namelen * 4 + 67.
> - */
> -
> - options[0].comment= name + namelen*2 + 10;
> - }
> + options[1].name= (options[0].name= name) + namelen + 1;
I'd suggest to remove the hack of allocating many 'name' in one call -
it's in a memroot anyway, there's no win here. Better:
options[0].name= (char*) alloc_root(mem_root, namelen);
options[1].name= (char*) alloc_root(mem_root, namelen + 8);
even better, use a constant instead of 8:
const LEX_STRING plugin_dash = { STRING_WITH_LEN("plugin-") };
...
options[1].name= (char*) alloc_root(mem_root, namelen + plugin_dash.length);
> + options[0].id= options[1].id= 256; /* must be >255. dup id ok */
> + options[0].var_type= options[1].var_type= GET_ENUM;
> + options[0].arg_type= options[1].arg_type= OPT_ARG;
> + options[0].def_value= options[1].def_value= 1; /* ON */
> + options[0].typelib= options[1].typelib= &global_plugin_typelib;
>
> /*
> - NOTE: 'name' is one char above the allocated buffer!
> - NOTE: This code assumes that 'my_bool' and 'char' are of same size.
> + NOTE: Make sure that comment doesn't become longer than comment_length
Why a "NOTE" ? What's wrong with strxnmov ?
> */
> - *((my_bool *)(name -1))= **enabled;
> - *enabled= (my_bool *)(name - 1);
> + strxmov(comment, "Enable or disable ", plugin_name, " plugin. "
> + "Disable with --skip-", name," (will save memory).", NullS);
> + options[0].comment= comment;
...
> @@ -3020,9 +3047,9 @@ static int construct_options(MEM_ROOT *m
> if (!opt->update)
> {
> opt->update= update_func_str;
> - if (!(opt->flags & PLUGIN_VAR_MEMALLOC | PLUGIN_VAR_READONLY))
> + if (!(opt->flags & (PLUGIN_VAR_MEMALLOC | PLUGIN_VAR_READONLY)))
> {
> - opt->flags|= PLUGIN_VAR_READONLY;
> + opt->flags|= (PLUGIN_VAR_READONLY);
I think this looks weird. Why the parentheses ?
> sql_print_warning("Server variable %s of plugin %s was forced "
> "to be read-only: string variable without "
> "update_func and PLUGIN_VAR_MEMALLOC flag",
> @@ -3208,64 +3241,80 @@ static int test_plugin_options(MEM_ROOT
> tmp->name.str);
> goto err;
> }
> + /*
> + Set plugin loading policy from option value. First element in the option
> + list is always the <plugin name> option value.
> + */
> + plugin_load_policy= (plugin_load_policy_t)(uint)*opts[0].value;
> + disable_plugin= (plugin_load_policy == PLUGIN_OFF);
> }
uhm, I think this is wrong. You have your default (disable_plugin)
behavior only if handle_options() wasn't called.
You need to set the default value (in opts[] array) from disable_plugin.
Either pass it down to construct_options() like the old code did or fix
the value after construct_options like in
opts[0].def_value= opts[1].def_value= disable_plugin;
> - if (!*enabled && !can_disable)
> + /*
> + The 'MyISAM' and 'Memory' storage engines currently can't be disabled.
> + TODO It really isn't possible to select --myisam=OFF as this option name
> + isn't registered.
why is it not registered ?
> + */
> + can_disable=
> + my_strcasecmp(&my_charset_latin1, tmp->name.str, "MyISAM") &&
> + my_strcasecmp(&my_charset_latin1, tmp->name.str, "MEMORY");
> +
> + tmp->is_mandatory= (plugin_load_policy == PLUGIN_FORCE);
> +
> + if (disable_plugin && !can_disable)
> {
> sql_print_warning("Plugin '%s' cannot be disabled", tmp->name.str);
> - *enabled= TRUE;
> + disable_plugin= FALSE;
> }
I like the old behavior, when --help message reflected the fact that the
engine is mandatory. Perhaps, again, either pass can_disable down to
construct_options, or fix the array afterwards:
if (!can_disable)
opts[0].comment= 0; // hide disabling option from --help
> - error= 1;
> -
> - if (*enabled)
> + /*
> + If the plugin is disabled it should not be initialized.
> + */
> + if (disable_plugin)
you print the warning even if global_system_variables.log_warnings is 0,
old code wasn't. Why the change ?
> {
> + sql_print_information("Plugin '%s' is disabled.",
> + tmp->name.str);
> + DBUG_RETURN(1);
by returning early you skip my_cleanup_options() call, which can cause
memoty leaks.
> + }
> +
> + error= 1;
> for (opt= tmp->plugin->system_vars; opt && *opt; opt++)
> {
> if (((o= *opt)->flags & PLUGIN_VAR_NOSYSVAR))
> continue;
> -
> if ((var= find_bookmark(tmp->name.str, o->name, o->flags)))
> v= new (mem_root) sys_var_pluginvar(var->key + 1, o);
> else
> {
> len= tmp->name.length + strlen(o->name) + 2;
> varname= (char*) alloc_root(mem_root, len);
> strxmov(varname, tmp->name.str, "-", o->name, NullS);
> my_casedn_str(&my_charset_latin1, varname);
> -
> for (p= varname; *p; p++)
> if (*p == '-')
> *p= '_';
> -
> v= new (mem_root) sys_var_pluginvar(varname, o);
> }
> DBUG_ASSERT(v); /* check that an object was actually constructed */
> -
> /*
> Add to the chain of variables.
> Done like this for easier debugging so that the
> pointer to v is not lost on optimized builds.
> */
> v->chain_sys_var(&chain);
> - }
> + } /* end for */
> if (chain.first)
> {
> chain.last->next = NULL;
> @@ -3281,11 +3314,7 @@ static int test_plugin_options(MEM_ROOT
> tmp->system_vars= chain.first;
> }
> DBUG_RETURN(0);
> - }
>
> - if (enabled_saved && global_system_variables.log_warnings)
> - sql_print_information("Plugin '%s' disabled by command line option",
> - tmp->name.str);
> err:
> if (opts)
> my_cleanup_options(opts);
>
Regards / Mit vielen Grüßen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Engineer/Server Architect
/_/ /_/\_, /___/\___\_\___/ Sun Microsystems GmbH, HRB München 161028
<___/ Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Häring
