Re: bzr commit into mysql-5.1-bugteam branch (ramil:2750) Bug#40757

[Ingo Strüwing]

Hi Ramil,

I agree with the patch, but would like to see some additional changes.
Please see below.

Ramil Kalimullin, 29.01.2009 18:03:

...
>  2750 Ramil Kalimullin	2009-01-29
>       Fix for bug #40757: Starting server on Windows with 
>       innodb_flush_method=wrong_value causes crash 
>       
>       Problem: failed plugin initialization (e.g. due to improper parameters)
>       may result in server crash.


This is a very dense description, which didn't help me to understand the
problem. Please add something like: "After the failed plugin
initialization, incompletely initialized data remained in the plugin and
handlerton data structures. These were used later and caused the crash".

...
> --- a/sql/handler.cc	2008-12-10 20:14:50 +0000
> +++ b/sql/handler.cc	2009-01-29 17:03:03 +0000
...
> @@ -494,6 +491,14 @@ int ha_initialize_handlerton(st_plugin_i
>          {
>            sql_print_error("Too many plugins loaded. Limit is %lu. "
>                            "Failed on '%s'", (ulong) MAX_HA, plugin->name.str);
> +          /* 
> +            Let's plugin do its inner deinitialization as plugin->init() 
> +            was successfully called above.
> +          */
> +          if (plugin->plugin->deinit)
> +          {
> +            (void) plugin->plugin->deinit(NULL);
> +          }
>            goto err;
...
>  err:
> +  my_free((uchar*) hton, MYF(0));
> +  plugin->data= NULL;

Good cleanup! However, I think, the same is required for the case of
"Too many storage engines!", where we just DBUG_RETURN(1).

And there is another small chance for problems. When we reach "Too many
plugins loaded", we did already do "installed_htons[hton->db_type]=
hton" and "savepoint_alloc_size+= tmp". Wouldn't it be better to undo
these too?

Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028