Hi, Kristofer!
On May 06, Kristofer Pettersson wrote:
>
> 2877 Kristofer Pettersson 2009-05-06
> Bug#19027 MySQL 5.0 starts even with Fatal InnoDB errors
Ok, I think I've found one more problem (re. --help) see below.
And a few style related comments, should be fixed too, but they're not
run-time bugs.
So that's almost final, I'm sure the next commit will be ok to push
> === 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-06 13:03:30 +0000
...
> 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 *optname, *p;
> + const LEX_STRING plugin_dash = { STRING_WITH_LEN((char *)"plugin-") };
there's a special define that allows you to avoid a cast here. You can
use either C_STRING_WITH_LEN or use a LEX_CSTRING instead of LEX_STRING.
I think the second one is better, as it avoids a cast completely.
> + uint plugin_name_len= strlen(plugin_name);
> + uint optnamelen;
> + const int max_comment_len= 120;
> + char *comment= (char *) alloc_root(mem_root, max_comment_len + 1);
> + char *optname;
> +
> int index= 0, offset= 0;
> st_mysql_sys_var *opt, **plugin_option;
> st_bookmark *v;
> - DBUG_ENTER("construct_options");
> - DBUG_PRINT("plugin", ("plugin: '%s' enabled: %d can_disable: %d",
> - plugin_name, **enabled, can_disable));
>
> - /* support --skip-plugin-foo syntax */
> - memcpy(name, plugin_name, namelen + 1);
> - my_casedn_str(&my_charset_latin1, name);
> - strxmov(name + namelen + 1, "plugin-", name, NullS);
> - /* Now we have namelen + 1 + 7 + namelen + 1 == namelen * 2 + 9. */
> + /** Used to circumvent the const attribute on my_option::name */
> + char *plugin_name_ptr, *plugin_name_with_prefix_ptr;
>
> - for (p= name + namelen*2 + 8; p > name; p--)
> - if (*p == '_')
> - *p= '-';
> + DBUG_ENTER("construct_options");
>
> - 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].name= plugin_name_ptr= (char*) alloc_root(mem_root,
> + plugin_name_len + 1);
> + strcpy(plugin_name_ptr, plugin_name);
> + my_casedn_str(&my_charset_latin1, plugin_name_ptr);
> + convert_underscore_to_dash(plugin_name_ptr, plugin_name_len);
> + /* support --skip-plugin-foo syntax */
> + options[1].name= plugin_name_with_prefix_ptr= (char*) alloc_root(mem_root,
> + plugin_name_len +
> + plugin_dash.length + 1);
> + strxmov(plugin_name_with_prefix_ptr, plugin_dash.str, options[0].name, NullS);
>
> - options[0].comment= name + namelen*2 + 10;
> - }
> + 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;
> +
> + strxnmov(comment, max_comment_len, "Enable or disable ", plugin_name,
> + " plugin. Disable with --skip-", options[0].name,
> + " (will save memory).", NullS);
> + options[0].comment= comment;
>
> /*
> - NOTE: 'name' is one char above the allocated buffer!
> - NOTE: This code assumes that 'my_bool' and 'char' are of same size.
> + Allocate temporary space for the value of the tristate.
> + This option will have a limited lifetime and is not used beyond
> + server initialization.
> */
> - *((my_bool *)(name -1))= **enabled;
> - *enabled= (my_bool *)(name - 1);
> -
> + options[0].value= options[1].value= (uchar **)alloc_root(mem_root,
> + sizeof(my_option **));
eh, why sizeof(my_option **) ?
It should be sizeof(int) with a comment /* GET_ENUM value is int */
> + *((uint*) options[0].value)= *((uint*) options[1].value)=
> + (uint) options[0].def_value;
>
> - options[1].name= (options[0].name= name) + namelen + 1;
> - options[0].id= options[1].id= 256; /* must be >255. dup id ok */
> - options[0].var_type= options[1].var_type= GET_BOOL;
> - options[0].arg_type= options[1].arg_type= NO_ARG;
> - options[0].def_value= options[1].def_value= **enabled;
> - options[0].value= options[0].u_max_value=
> - options[1].value= options[1].u_max_value= (uchar**) (name - 1);
> options+= 2;
>
> /*
> @@ -3208,64 +3257,94 @@ 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;
enum_plugin_load_policy would be more in line with the current coding
style (for enum names) than plugin_load_policy_t.
> }
>
> - if (!*enabled && !can_disable)
> + disable_plugin= (plugin_load_policy == PLUGIN_OFF);
> + /*
> + 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.
as we've discussed on IRC --myisam=OFF doesn't work for a different
reason, --myisam is registered. Please remove this "TODO" part of the
comment.
> + */
> + 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) || !can_disable;
> +
> + if (disable_plugin && !can_disable)
> {
> sql_print_warning("Plugin '%s' cannot be disabled", tmp->name.str);
> - *enabled= TRUE;
> + disable_plugin= FALSE;
> }
>
> - error= 1;
> -
> - if (*enabled)
> + /*
> + If the plugin is configured as MYSQL_PLUGIN_MANDATORY() in plug.in then
> + it can't be disabled. To avoid printing usage information stating that
> + it is possible to disable the plugin the comment line is removed.
> + */
> + if (!can_disable && opts)
> {
> - 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= '_';
> + opts[0].comment= 0;
> + opts[1].comment= 0;
> + }
Eh, did you check that it works ?
I don't see a point of removing a comment in test_plugin_options() after
handle_options was called :)
It should be done in construct_help_options(), perhaps.
> - v= new (mem_root) sys_var_pluginvar(varname, o);
> - }
> - DBUG_ASSERT(v); /* check that an object was actually constructed */
> + /*
> + If the plugin is disabled it should not be initialized.
> + */
> + if (disable_plugin)
> + {
> + if (global_system_variables.log_warnings)
> + sql_print_information("Plugin '%s' is disabled.",
> + tmp->name.str);
> + if (opts)
> + my_cleanup_options(opts);
> + DBUG_RETURN(1);
> + }
>
> - /*
> - 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);
> + 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= '_';
could you move the above to an inline function too ?
> + v= new (mem_root) sys_var_pluginvar(varname, o);
> }
> - if (chain.first)
> + 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);
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, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring
