← Back to team overview

maria-developers team mailing list archive

Re: Rev 2734: Maria WL#61

 

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, 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 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_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);
+#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




Follow ups

References