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
start
on this one until you're ready with wl:43 and fixed the soft_sync_rwl
problem in 5.2.
On Mar 11, [email protected] 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 0
make 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
start
with 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 letter
or 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_plu...@mysql_plugin_defs@;
-
-struct st_mysql_plugin *mysqld_builtins[]=
+typedef struct st_maria_plugin builtin_maria_plugin[];
+
+extern builtin_maria_plugin
+ builtin_maria_binlog_plu...@maria_plugin_defs@;
+
+struct st_maria_plugin *mariadb_builtins[]=
{
- builtin_binlog_plu...@mysql_plugin_defs@,(struct st_mysql_plugin
*)0
+ builtin_maria_binlog_plu...@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);
+#endif
I 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
interpreted
unambiguously.
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;
+#endif
Paul 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
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
