From: Date: January 23 2008 5:33pm
Subject: Re: bk commit into 5.1 tree (acurtis:1.2655) BUG#33358
List-Archive: http://lists.mysql.com/commits/41157
Message-Id: <7C0BD734-9449-45A7-9012-BC3E1584E1BF@mysql.com>
MIME-Version: 1.0 (Apple Message framework v915)
Content-Type: text/plain; charset=ISO-8859-1; format=flowed; delsp=yes
Content-Transfer-Encoding: quoted-printable

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 =20
>> command line.
>>    fix off-by-one bug when loading multiple plug-ins from the =20
>> command line.
>>    initialize command line handling for ENUM and SET plugin =20
>> variable types.
>
>> diff -Nrup a/mysql-test/r/plugin_bug33358.result b/mysql-test/r/=20
>> 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 =3D 'e2';
>> +@@global.example_enum_var =3D 'e2'
>> +1
>> diff -Nrup a/mysql-test/t/plugin_bug33358-master.opt b/mysql-test/t/=20=

>> 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 =20
>> -08:00
>> @@ -0,0 +1,3 @@
>> +$EXAMPLE_PLUGIN_OPT
>> +--plugin-load=3DEXAMPLE=3Dha_example.so::
>> +--plugin-example-enum-var=3De2
>> diff -Nrup a/mysql-test/t/plugin_bug33358.test b/mysql-test/t/=20
>> 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 =3D '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 =20
tests... I suppose I can just ass it to existing plugin test and fixup =20=

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 =3D=3D 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 =20
> that.
>
> please remove this unlock and add it before every goto error.

done,

>
>>   sql_print_error("Couldn't load plugin named '%s' with soname =20
>> '%s'.",
>>                   name.str, dl.str);
>>   DBUG_RETURN(TRUE);
>> @@ -3017,7 +3030,22 @@ static int construct_options(MEM_ROOT *m
>>       optnamelen=3D namelen + optnamelen + 1;
>>     }
>>     else
>> -      optname=3D (char*) memdup_root(mem_root, v->key + 1, =20
>> (optnamelen=3D v->name_len) + 1);
>> +    {
>> +      if (!(v=3D 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 =20=

register_var failed to insert into the hash. Would it be preferred if =20=

I remove the 'if' and simply put an assert?

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


Regards,
Antony.=