Sergei Golubchik wrote:
>> +/**
>> + @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)
>
OK
>> 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
>
OK. I fail to see the point in any of the calculations actually. I'm
changing this to a fixed length. I think max 120 characters would be
suitable for comments.
>> - }
>> + 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);
>
Totally agree.
>> + 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 ?
OK! I was totally blind of the fact that strxnmov() existed and I just
refactored the existing 'NOTE'. I'll happily use strxnmov() instead.
>> + opt->flags|= (PLUGIN_VAR_READONLY);
>
> I think this looks weird. Why the parentheses ?
It removed a warning from GCC.
>
>> 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.
Yes this is strange. I wonder how I got away with this during testing...
Fixing.
>
> 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 ?
MyISAM is not part of the plugins which we iterate over in
plugin_init(). Perhaps it should?
>
>> + */
>> + 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
>
I think I disagree. Please help me understand what you mean.
Strictly speaking all plugins can now be mandatory, that's why I've
writing this patch (I think. :))
How are help texts affected by comment== 0? The comment in
my_print_help_inc_plugins() says "Only options with a non-NULL comment
are displayed in help text". To me this means that if there is a
comment, it should be displayed. I don't see an explicit intention to
restrict help text to "mandatory plugins". Further more the comment to
the caller usage() states:
"/* Print out all the options including plugin supplied options */"
Again "all" and "including".
>> - 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 ?
>
I made the change because I strongly believe that loose and implicit
connection between variable states are the root of all evil. If I see a
predicate I want to as quickly as possible deduce its plausibility
instead of having to reverse engineer the code to find all possible states.
However, I perhaps wasn't careful when I made this change. Can you
explain to me what the connection is between
global_system_variables.log_warnings and disabled plugins?
Personally I think it makes much sense to always print some kind of
message when a plugin is loaded, initialized, enabled, disabled or
uninitialized.
>> {
>> + 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.
Could you help me see where the memory leak happens?
The assumption is that the options are allocated on a temporary memory
root. It also seems like my_cleanup_options() is a wrapper around
init_variables() which sets the default values for the options. What has
that to do with memory clean up?
