| Thread Previous • Date Previous • Date Next • Thread Next |
Hi, Serg! 23 марта 2010, в 21:53, Sergei Golubchik написал(а):
Hi, Sanja! See the review below.But please note that wl:43 and red 5.2 have higher priority, don't starton this one until you're ready with wl:43 and fixed the soft_sync_rwl problem in 5.2. On Mar 11, sanja@xxxxxxxxxxxx wrote:=== modified file 'include/mysql/plugin.h' --- a/include/mysql/plugin.h 2009-09-07 20:50:10 +0000 +++ b/include/mysql/plugin.h 2010-03-11 15:02:03 +0000 @@ -86,6 +89,21 @@ #define PLUGIN_LICENSE_GPL_STRING "GPL" #define PLUGIN_LICENSE_BSD_STRING "BSD" +/* definitions of code maturity for plugins */ +#define PLUGIN_MATURITY_UNKNOWN 0make it MariaDB_PLUGIN_MATURITY_...
OK
+#define PLUGIN_MATURITY_TEST 1 +#define PLUGIN_MATURITY_ALPHA 2 +#define PLUGIN_MATURITY_BETA 3 +#define PLUGIN_MATURITY_GAMMA 4 +#define PLUGIN_MATURITY_RELEASE 5"release" ? or production or stable ? there can be an alpha release, or beta release. obviously, "release" does not indicate a degree of maturity. and I don't like "test" either.PLUGIN_MATURITY_TEST is just not readable. I'd rather drop it and startwith alpha.
I agree with changing RELEASE with STABLE, but test looks logical for me (and for Monty also as far he invented it).
+ +#define PLUGIN_MATURITY_UNKNOWN_STR "Unknown" +#define PLUGIN_MATURITY_TEST_STR "Test" +#define PLUGIN_MATURITY_ALPHA_STR "Alpha" +#define PLUGIN_MATURITY_BETA_STR "Beta" +#define PLUGIN_MATURITY_GAMMA_STR "Gamma" +#define PLUGIN_MATURITY_RELEASE_STR "Release" +why did you put these macros in the plugin.h ?as far as I can see there's no need to make them part of the interface,they can be completely internal
OK
/* Macros for beginning and ending plugin declarations. Between mysql_declare_plugin and mysql_declare_plugin_end there should @@ -94,15 +112,29 @@ #ifndef MYSQL_DYNAMIC_PLUGIN +#define __MYSQL_DECLARE_PLUGIN(NAME, VERSION, PSIZE, DECLS) \ int VERSION= MYSQL_PLUGIN_INTERFACE_VERSION; \ int PSIZE= sizeof(struct st_mysql_plugin); \struct st_mysql_plugin DECLS[]= { ++#define __MARIA_DECLARE_PLUGIN(NAME, VERSION, PSIZE, DECLS) \don't use names that start from __, the C99 standard says"All identifiers that begin with an underscore and either an uppercase letteror another underscore are always reserved for any use." (ISO/IEC 9899:1999, 7.1.3 Reserved Identifiers) It's unfortunate that __MYSQL_DECLARE_PLUGIN breaks this rule, but it's not a reason for us to do the same
So what will be better, one or three or remove '_' at all?
+int VERSION= MARIA_PLUGIN_INTERFACE_VERSION; \ +int PSIZE= sizeof(struct st_maria_plugin); \+struct st_maria_plugin DECLS[]= { + #else +#define __MYSQL_DECLARE_PLUGIN(NAME, VERSION, PSIZE, DECLS) \ MYSQL_PLUGIN_EXPORT int _mysql_plugin_interface_version_= MYSQL_PLUGIN_INTERFACE_VERSION; \ MYSQL_PLUGIN_EXPORT int _mysql_sizeof_struct_st_plugin_= sizeof(struct st_mysql_plugin); \ MYSQL_PLUGIN_EXPORT struct st_mysql_plugin _mysql_plugin_declarations_[]= {++#define __MARIA_DECLARE_PLUGIN(NAME, VERSION, PSIZE, DECLS) \ +MYSQL_PLUGIN_EXPORT int _maria_plugin_interface_version_= MARIA_PLUGIN_INTERFACE_VERSION; \ +MYSQL_PLUGIN_EXPORT int _maria_sizeof_struct_st_plugin_= sizeof(struct st_maria_plugin); \ +MYSQL_PLUGIN_EXPORT struct st_maria_plugin _maria_plugin_declarations_[]= {+ #endif #define mysql_declare_plugin(NAME) \ @@ -407,6 +446,31 @@void * __reserved1; /* reserved for dependency checking */}; +/* + MariaDB extension for plugins declaration structure. + + It also copy current MySQL plugin fields to have more independency + in plugins extension +*/ + +struct st_maria_plugin +{+ int type; /* the plugin type (a MYSQL_XXX_PLUGIN value) */ + void *info; /* pointer to type-specific plugin descriptor */ + const char *name; /* plugin name */ + const char *author; /* plugin author (for SHOW PLUGINS) */ + const char *descr; /* general descriptive text (for SHOW PLUGINS ) */ + int license; /* the plugin license (PLUGIN_LICENSE_XXX) */ + int (*init)(void *); /* the function to invoke when plugin is loaded */ + int (*deinit)(void *);/* the function to invoke when plugin is unloaded */ + unsigned int version; /* plugin version (for SHOW PLUGINS) */+ struct st_mysql_show_var *status_vars; + struct st_mysql_sys_var **system_vars; + const char *version_info; /* plugin version string */ + int maturity; /* HA_PLUGIN_MATURITY_XXX */+ void * __reserved1; /* reserved for dependency checking */remove this __reserved1 field
OK
+}; +/ *************************************************************************API for Full-text parser plugin. (MYSQL_FTPARSER_PLUGIN) */ === modified file 'sql/ha_ndbcluster.cc' --- a/sql/ha_ndbcluster.cc 2009-09-07 20:50:10 +0000 +++ b/sql/ha_ndbcluster.cc 2010-03-11 15:02:03 +0000 @@ -10561,5 +10561,23 @@ NULL /* config options */ } mysql_declare_plugin_end; +maria_declare_plugin(ndbcluster) +{ + MYSQL_STORAGE_ENGINE_PLUGIN, + &ndbcluster_storage_engine, + ndbcluster_hton_name, + "MySQL AB", + "Clustered, fault-tolerant tables", + PLUGIN_LICENSE_GPL, + ndbcluster_init, /* Plugin Init */ + NULL, /* Plugin Deinit */ + 0x0100 /* 1.0 */, + ndb_status_variables_export,/* status variables */ + NULL, /* system variables */ + "1.0", /* string version */ + PLUGIN_MATURITY_BETA, /* maturity */beta ? says who ?
My guess reviewed by Monty.
+ NULL /* config options */ +} +maria_declare_plugin_end; #endif === modified file 'sql/sql_builtin.cc.in' --- a/sql/sql_builtin.cc.in 2006-12-31 01:29:11 +0000 +++ b/sql/sql_builtin.cc.in 2010-03-11 15:02:03 +0000 @@ -15,13 +15,12 @@ #include <mysql/plugin.h> -typedef struct st_mysql_plugin builtin_plugin[]; - -extern builtin_plugin - builtin_binlog_plugin@mysql_plugin_defs@; - -struct st_mysql_plugin *mysqld_builtins[]= +typedef struct st_maria_plugin builtin_maria_plugin[]; + +extern builtin_maria_plugin + builtin_maria_binlog_plugin@maria_plugin_defs@; + +struct st_maria_plugin *mariadb_builtins[]= {- builtin_binlog_plugin@mysql_plugin_defs@,(struct st_mysql_plugin *)0 + builtin_maria_binlog_plugin@maria_plugin_defs@,(struct st_maria_plugin *)0}; -did you have to rename these structures ?
I thought it would be better as far as they differ from MySQL ones.
=== modified file 'sql/sql_plugin.cc' --- a/sql/sql_plugin.cc 2009-11-12 04:31:28 +0000 +++ b/sql/sql_plugin.cc 2010-03-11 15:02:03 +0000 @@ -341,11 +349,261 @@ dlclose(p->handle); #endif my_free(p->dl.str, MYF(MY_ALLOW_ZERO_PTR)); - if (p->version != MYSQL_PLUGIN_INTERFACE_VERSION) + if (p->mariaversion != MARIA_PLUGIN_INTERFACE_VERSION) my_free((uchar*)p->plugins, MYF(MY_ALLOW_ZERO_PTR)); } +/** + Reads data from mysql plugin interface + + @param plugin_dl Structure where the data should be put + @param sym Reverence on version info + @param dlpath Path to the module + @param report What errors should be reported + + @retval FALSE OK + @retval TRUE ERROR +*/ ++static my_bool read_mysql_plugin_info(struct st_plugin_dl *plugin_dl,+ void *sym, char *dlpath, + int report) +{ + DBUG_ENTER("read_maria_plugin_info"); + /* Determine interface version */ + if (!sym) + { + free_plugin_mem(plugin_dl); + if (report & REPORT_TO_USER)+ my_error(ER_CANT_FIND_DL_ENTRY, MYF(0), plugin_interface_version_sym);+ if (report & REPORT_TO_LOG)+ sql_print_error(ER(ER_CANT_FIND_DL_ENTRY), plugin_interface_version_sym);+ DBUG_RETURN(TRUE);this needs to be fixed to match the latest code in the pluggable- auth branch. (where I've backported few bugfixes and small cleanups from mysql- next)in particular * don't use my_error()/sql_print_error(), use report_error(). * fix the bug, mentioned below
I saw your bugfix but left it for merge, usually it is not good to fix the same things in different paches
+ } + plugin_dl->mariaversion= 0; + plugin_dl->mysqlversion= *(int *)sym; + /* Versioning */ + if (plugin_dl->mysqlversion < min_plugin_interface_version ||+ (plugin_dl->mysqlversion >> 8) > (MYSQL_PLUGIN_INTERFACE_VERSION >> 8))+ { + free_plugin_mem(plugin_dl); + if (report & REPORT_TO_USER) + my_error(ER_CANT_OPEN_LIBRARY, MYF(0), dlpath, 0, + "plugin interface version mismatch"); + if (report & REPORT_TO_LOG) + sql_print_error(ER(ER_CANT_OPEN_LIBRARY), dlpath, 0, + "plugin interface version mismatch"); + DBUG_RETURN(TRUE); + } + /* Find plugin declarations */ + if (!(sym= dlsym(plugin_dl->handle, plugin_declarations_sym))) + { + free_plugin_mem(plugin_dl); + if (report & REPORT_TO_USER)+ my_error(ER_CANT_FIND_DL_ENTRY, MYF(0), plugin_declarations_sym);+ if (report & REPORT_TO_LOG)+ sql_print_error(ER(ER_CANT_FIND_DL_ENTRY), plugin_declarations_sym);+ DBUG_RETURN(TRUE); + } + + /* convert mysql declaration to maria one */ + { + int i; + uint sizeof_st_plugin; + struct st_mysql_plugin *old; + struct st_maria_plugin *cur; + char *ptr= (char *)sym; + + if ((sym= dlsym(plugin_dl->handle, sizeof_st_plugin_sym))) + sizeof_st_plugin= *(int *)sym; + else + { +#ifdef ERROR_ON_NO_SIZEOF_PLUGIN_SYMBOL + free_plugin_mem(plugin_dl); + if (report & REPORT_TO_USER)+ my_error(ER_CANT_FIND_DL_ENTRY, MYF(0), sizeof_st_plugin_sym);+ if (report & REPORT_TO_LOG)+ sql_print_error(ER(ER_CANT_FIND_DL_ENTRY), sizeof_st_plugin_sym);+ DBUG_RETURN(TRUE); +#else + /*+ When the following assert starts failing, we'll have to switch+ to the upper branch of the #ifdef + */ + DBUG_ASSERT(min_plugin_interface_version == 0);+ sizeof_st_plugin= (int)offsetof(struct st_mysql_plugin, version);+#endifI suppose you can remove this #ifdef now.
I removed it in our function but left code of mysql plugin reading as is. If you think that it would be better to remove, it is easy.
+ } + + for (i= 0; + ((struct st_mysql_plugin *)(ptr+i*sizeof_st_plugin))->info; + i++) + /* no op */; + + cur= (struct st_maria_plugin*) + my_malloc(i * sizeof(struct st_maria_plugin),it's a bug, which I've fixed just recently. See http://bazaar.launchpad.net/~maria-captains/maria/5.2-pluggable-auth/revision/2643.11.166
I saw the bug, but left it for merge.
+ MYF(MY_ZEROFILL|MY_WME)); + if (!cur) + { + free_plugin_mem(plugin_dl); + if (report & REPORT_TO_USER) + my_error(ER_OUTOFMEMORY, MYF(0), plugin_dl->dl.length); + if (report & REPORT_TO_LOG) + sql_print_error(ER(ER_OUTOFMEMORY), plugin_dl->dl.length); + DBUG_RETURN(TRUE); + } + /*+ All st_plugin fields not initialized in the plugin explicitly, are + set to 0. It matches C standard behaviour for struct initializers that+ have less values than the struct definition. + */ + for (i=0;+ (old=(struct st_mysql_plugin *)(ptr+i*sizeof_st_plugin))- >info;+ i++) + { + + cur->type= old->type; + cur->info= old->info; + cur->name= old->name; + cur->author= old->author; + cur->descr= old->descr; + cur->license= old->license; + cur->init= old->init; + cur->deinit= old->deinit; + cur->version= old->version; + cur->status_vars= old->status_vars; + cur->system_vars= old->system_vars; + /* + Something like this should be added to process + new mysql plugin versions: + if (plugin_dl->mysqlversion > 0x0100) + { + cur->newfield= CONSTANT_MEANS_UNKNOWN; + } + else + { + cur->newfield= old->newfield; + } + */ + /* Maria only fields */ + cur->version_info= "Unknown"; + cur->maturity= PLUGIN_MATURITY_UNKNOWN; + } + + plugin_dl->plugins= (struct st_maria_plugin *)cur; + } + + DBUG_RETURN(FALSE); +} + + +/** + Reads data from maria plugin interface + + @param plugin_dl Structure where the data should be put + @param sym Reverence on version info + @param dlpath Path to the module + @param report what errors should be reported + + @retval FALSE OK + @retval TRUE ERROR +*/ ++static my_bool read_maria_plugin_info(struct st_plugin_dl *plugin_dl,+ void *sym, char *dlpath, + int report) +{ + DBUG_ENTER("read_maria_plugin_info"); + + /* Determine interface version */ + if (!(sym)) + { + free_plugin_mem(plugin_dl); + if (report & REPORT_TO_USER)+ my_error(ER_CANT_FIND_DL_ENTRY, MYF(0), plugin_interface_version_sym);+ if (report & REPORT_TO_LOG)+ sql_print_error(ER(ER_CANT_FIND_DL_ENTRY), plugin_interface_version_sym);+ DBUG_RETURN(TRUE); + } + plugin_dl->mariaversion= *(int *)sym; + plugin_dl->mysqlversion= 0; + /* Versioning */+ if (plugin_dl->mariaversion < min_maria_plugin_interface_version || + (plugin_dl->mariaversion >> 8) > (MARIA_PLUGIN_INTERFACE_VERSION >> 8))+ { + free_plugin_mem(plugin_dl); + if (report & REPORT_TO_USER) + my_error(ER_CANT_OPEN_LIBRARY, MYF(0), dlpath, 0, + "plugin interface version mismatch"); + if (report & REPORT_TO_LOG) + sql_print_error(ER(ER_CANT_OPEN_LIBRARY), dlpath, 0, + "plugin interface version mismatch"); + DBUG_RETURN(TRUE); + } + /* Find plugin declarations */+ if (!(sym= dlsym(plugin_dl->handle, maria_plugin_declarations_sym)))+ { + free_plugin_mem(plugin_dl); + if (report & REPORT_TO_USER)+ my_error(ER_CANT_FIND_DL_ENTRY, MYF(0), plugin_declarations_sym);+ if (report & REPORT_TO_LOG)+ sql_print_error(ER(ER_CANT_FIND_DL_ENTRY), plugin_declarations_sym);+ DBUG_RETURN(TRUE); + } + if (plugin_dl->mariaversion != MARIA_PLUGIN_INTERFACE_VERSION) + { + int i; + uint sizeof_st_plugin; + struct st_maria_plugin *old, *cur; + char *ptr= (char *)sym; + + if ((sym= dlsym(plugin_dl->handle, maria_sizeof_st_plugin_sym))) + sizeof_st_plugin= *(int *)sym; + else + { + free_plugin_mem(plugin_dl); + if (report & REPORT_TO_USER)+ my_error(ER_CANT_FIND_DL_ENTRY, MYF(0), sizeof_st_plugin_sym);+ if (report & REPORT_TO_LOG)+ sql_print_error(ER(ER_CANT_FIND_DL_ENTRY), sizeof_st_plugin_sym);+ DBUG_RETURN(TRUE); + } + + for (i= 0; + ((struct st_maria_plugin *)(ptr+i*sizeof_st_plugin))->info; + i++) + /* no op */; + + cur= (struct st_maria_plugin*) + my_malloc(i * sizeof(struct st_maria_plugin), + MYF(MY_ZEROFILL|MY_WME)); + if (!cur) + { + free_plugin_mem(plugin_dl); + if (report & REPORT_TO_USER) + my_error(ER_OUTOFMEMORY, MYF(0), plugin_dl->dl.length); + if (report & REPORT_TO_LOG) + sql_print_error(ER(ER_OUTOFMEMORY), plugin_dl->dl.length); + DBUG_RETURN(TRUE); + } + /*+ All st_plugin fields not initialized in the plugin explicitly, are + set to 0. It matches C standard behaviour for struct initializers that+ have less values than the struct definition. + */ + for (i=0;+ (old=(struct st_maria_plugin *)(ptr+i*sizeof_st_plugin))- >info;+ i++) + memcpy(cur+i, old, min(sizeof(cur[i]), sizeof_st_plugin)); + + sym= cur; + } + plugin_dl->plugins= (struct st_maria_plugin *)sym; + + DBUG_RETURN(FALSE); +}I don't know, two almost identical functions - this doesn't look very nice.can you try to reduce the amount of code duplication ?
The structures are different and could be more different. All ways which I come up with looked like hacks (like type casting for same parts of structures) so I decided better to keep it in different functions.
static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report) { #ifdef HAVE_DLOPEN === modified file 'sql/sql_show.cc' --- a/sql/sql_show.cc 2009-11-12 04:31:28 +0000 +++ b/sql/sql_show.cc 2010-03-11 15:02:03 +0000 @@ -143,7 +151,9 @@ table->field[5]->set_notnull(); table->field[6]->store(version_buf, make_version_string(version_buf, sizeof(version_buf), - plugin_dl->version), + (plugin_dl->mariaversion ? + plugin_dl->mariaversion : + plugin_dl->mysqlversion)),okay, although I'd probably just print plugin_dl->mariaversion here.if it's 0 - then it's 0. Otherwise the version number cannot be interpretedunambiguously.
OK
cs); table->field[6]->set_notnull(); } === modified file 'storage/pbxt/src/ha_pbxt.cc' --- a/storage/pbxt/src/ha_pbxt.cc 2009-09-03 06:15:03 +0000 +++ b/storage/pbxt/src/ha_pbxt.cc 2010-03-11 15:02:03 +0000 @@ -5507,6 +5507,42 @@ drizzle_declare_plugin_end; #else mysql_declare_plugin_end; +#ifdef MARIADB_BASE_VERSION +maria_declare_plugin(pbxt) +{ /* PBXT */ + MYSQL_STORAGE_ENGINE_PLUGIN, + &pbxt_storage_engine, + "PBXT", + "Paul McCullagh, PrimeBase Technologies GmbH", + "High performance, multi-versioning transactional engine", + PLUGIN_LICENSE_GPL, + pbxt_init, /* Plugin Init */ + pbxt_end, /* Plugin Deinit */ + 0x0001 /* 0.1 */, + NULL, /* status variables */ + pbxt_system_variables, /* system variables */ + "1.0.09g RC3", /* string version */ + PLUGIN_MATURITY_GAMMA, /* maturity */ + NULL /* config options */ +}, +{ /* PBXT_STATISTICS */ + MYSQL_INFORMATION_SCHEMA_PLUGIN, + &pbxt_statitics, + "PBXT_STATISTICS", + "Paul McCullagh, PrimeBase Technologies GmbH", + "PBXT internal system statitics", + PLUGIN_LICENSE_GPL, + pbxt_init_statitics, /* plugin init */ + pbxt_exit_statitics, /* plugin deinit */ + 0x0005, + NULL, /* status variables */ + NULL, /* system variables */ + "1.0.09g RC3", /* string version */ + PLUGIN_MATURITY_GAMMA, /* maturity */ + NULL /* config options */ +} +maria_declare_plugin_end; +#endifPaul needs to know about this to update his repository
I think letter is enough, right?
#endif #if defined(XT_WIN) && defined(XT_COREDUMP)Regards, Sergei
| Thread Previous • Date Previous • Date Next • Thread Next |
