Sergei Golubchik wrote:
>> - 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.
>
OK. I can't find LEX_CSTRING in 5.1-branch so I'm going with
C_STRING_WITH_LEN (unless you want me to back port it with this patch?).
>> + 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 */
I don't remember :(
Your suggestion looks better.
>> + /*
>> + 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.
>
OK.
>> }
>>
>> - 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.
>
OK.
>> + 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.
>
Yup, it is all wrong. I implemented half-of-an-idea here for your proposal:
>> 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 suppose I was confused by the fact that MYSQL_PLUGIN_MANDATORY(maria)
does nothing about whether a plugin is mandatory or not in this context.
Neither MyISAM nor Memory storage engines seems to print any help texts
which needs to be fixed. So in that regard it "worked" when I tested. :^)
I propose that we wait with fixing this issue until
MYSQL_PLUGIN_MANDATORY is fixed. See http://bugs.mysql.com/bug.php?id=44691
I'm removing the comment + the two null assignments for now.
>> + 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 ?
>
OK.
