Re: bk commit into 5.1 tree (acurtis:1.2655) BUG#33358

[Antony T Curtis]

Hi Sergei,

New cset committed.


On 22 Jan, 2008, at 11:57, Sergei Golubchik wrote:

> Hi!
>
> On Jan 21, antony@stripped wrote:
>> ChangeSet@stripped, 2008-01-21 12:01:21-08:00, acurtis@stripped +4 -0
>>  Bug#33358
>>    "Plugin enum variables can't be set from command line"
>>    fix crash of LOCK_plugins mutex when loading plug-ins from  
>> command line.
>>    fix off-by-one bug when loading multiple plug-ins from the  
>> command line.
>>    initialize command line handling for ENUM and SET plugin  
>> variable types.
>
>> diff -Nrup a/mysql-test/r/plugin_bug33358.result b/mysql-test/r/ 
>> plugin_bug33358.result
>> --- /dev/null	Wed Dec 31 16:00:00 196900
>> +++ b/mysql-test/r/plugin_bug33358.result	2008-01-21 12:01:15 -08:00
>> @@ -0,0 +1,3 @@
>> +SELECT @@global.example_enum_var = 'e2';
>> +@@global.example_enum_var = 'e2'
>> +1
>> diff -Nrup a/mysql-test/t/plugin_bug33358-master.opt b/mysql-test/t/ 
>> plugin_bug33358-master.opt
>> --- /dev/null	Wed Dec 31 16:00:00 196900
>> +++ b/mysql-test/t/plugin_bug33358-master.opt	2008-01-21 12:01:15  
>> -08:00
>> @@ -0,0 +1,3 @@
>> +$EXAMPLE_PLUGIN_OPT
>> +--plugin-load=EXAMPLE=ha_example.so::
>> +--plugin-example-enum-var=e2
>> diff -Nrup a/mysql-test/t/plugin_bug33358.test b/mysql-test/t/ 
>> plugin_bug33358.test
>> --- /dev/null	Wed Dec 31 16:00:00 196900
>> +++ b/mysql-test/t/plugin_bug33358.test	2008-01-21 12:01:15 -08:00
>> @@ -0,0 +1,3 @@
>> +--source include/have_example_plugin.inc
>> +
>> +SELECT @@global.example_enum_var = 'e2';
>
> perhaps you could add this test to an existing test file that already
> requires ha_example.so ? One restart less for the test suite...

Choices are limited and doing this would alter results from other  
tests... I suppose I can just ass it to existing plugin test and fixup  
changed results accordingly.

>
>> diff -Nrup a/sql/sql_plugin.cc b/sql/sql_plugin.cc
>> --- a/sql/sql_plugin.cc	2007-12-13 03:49:55 -08:00
>> +++ b/sql/sql_plugin.cc	2008-01-21 12:01:15 -08:00
>> @@ -1412,7 +1412,11 @@ static bool plugin_load_list(MEM_ROOT *t
>>   while (list)
>>   {
>>     if (p == buffer + sizeof(buffer) - 1)
>> -      break;
>> +	{
>
> use spaces, not tabs

fixed,

>
>
>> @@ -1464,6 +1475,7 @@ static bool plugin_load_list(MEM_ROOT *t
>>   }
>>   DBUG_RETURN(FALSE);
>> error:
>> +  pthread_mutex_unlock(&LOCK_plugin);
>
> uhm, I don't know. better to unlock in the mutex in the same case: of
> switch() where you locked it. I agree that currently this case: is the
> only place where you do 'goto error', but it's fragile to rely on  
> that.
>
> please remove this unlock and add it before every goto error.

done,

>
>>   sql_print_error("Couldn't load plugin named '%s' with soname  
>> '%s'.",
>>                   name.str, dl.str);
>>   DBUG_RETURN(TRUE);
>> @@ -3017,7 +3030,22 @@ static int construct_options(MEM_ROOT *m
>>       optnamelen= namelen + optnamelen + 1;
>>     }
>>     else
>> -      optname= (char*) memdup_root(mem_root, v->key + 1,  
>> (optnamelen= v->name_len) + 1);
>> +    {
>> +      if (!(v= find_bookmark(name, opt->name, opt->flags)))
>> +      {
>> +        sql_print_error("Thread local variable '%s' not allocated "
>> +                        "in plugin '%s'.", opt->name, plugin_name);
>
> why could that happen ?

This is a very unlikely event and would only occur if for some reason  
register_var failed to insert into the hash. Would it be preferred if  
I remove the 'if' and simply put an assert?

>
>> +    	DBUG_RETURN(-1);
>> +      }
>> +
>> +      *(int*)(opt + 1)= offset= v->offset;
>> +
>> +      if (opt->flags & PLUGIN_VAR_NOCMDOPT)
>> +        continue;
>> +
>> +      optname= (char*) memdup_root(mem_root, v->key + 1,
>> +                                   (optnamelen= v->name_len) + 1);
>> +    }
>>
>>     /* convert '_' to '-' */
>>     for (p= optname; *p; p++)
>
> Regards / Mit vielen Grüssen,
> Sergei
>
>


Regards,
Antony.