Hi Magnus,
Thanks for fixing this. The patch is mostly ok, but would you mind doing
the following:
- I think binlog_func_foreach should return int, because both the
functions that call it and the functions it calls return int, and the
value is propagated to callers.
- Since you do this change now, would you mind doing the same to
ha_binlog_end?
/Sven
On 02/01/2011 10:48 AM, Magnus Blåudd wrote:
> #At file:///home/msvensson/mysql/trunk-bug59844/ based on
> revid:jon.hauglid@stripped
>
> 3580 Magnus Blåudd 2011-02-01
> Bug#59844 Return value of ndb binlog functions are not checked
> - return error code from ha_reset_slave, ha_reset_logs, ha_binlog_end and
> ha_binlog_index_purge_file functions.
> - check and handler return code from ha_reset_logs and ha_reset_slave
> - Since ha_binlog_end is called during shutdown there is no error handling
> to perform, the function should have been void.
> - The ha_binlog_index_purge_file function might be necessary to handle
> the return code from.
>
> modified:
> sql/binlog.cc
> sql/handler.cc
> sql/handler.h
> sql/rpl_slave.cc
> === modified file 'sql/binlog.cc'
> --- a/sql/binlog.cc 2010-12-17 02:01:32 +0000
> +++ b/sql/binlog.cc 2011-02-01 09:48:52 +0000
> @@ -2130,7 +2130,18 @@ bool MYSQL_BIN_LOG::reset_logs(THD* thd)
> const char* save_name;
> DBUG_ENTER("reset_logs");
>
> - ha_reset_logs(thd);
> + if (ha_reset_logs(thd))
> + {
> + /*
> + Failed to reset logs in handler(which should have pushed warnings
> + describing the problem in more detail) -> raise fatal binlog
> + purge error
> + */
> + thd->raise_error(ER_BINLOG_PURGE_FATAL_ERR);
> +
> + DBUG_RETURN(1);
> + }
> +
> /*
> We need to get both locks to be sure that no one is trying to
> write to the index log file.
>
> === modified file 'sql/handler.cc'
> --- a/sql/handler.cc 2011-01-21 10:50:31 +0000
> +++ b/sql/handler.cc 2011-02-01 09:48:52 +0000
> @@ -4231,27 +4231,28 @@ static my_bool binlog_func_foreach(THD *
> {
> hton_list_st hton_list;
> uint i, sz;
> + int res= 0;
>
> hton_list.sz= 0;
> plugin_foreach(thd, binlog_func_list,
> MYSQL_STORAGE_ENGINE_PLUGIN,&hton_list);
>
> for (i= 0, sz= hton_list.sz; i< sz ; i++)
> - hton_list.hton[i]->binlog_func(hton_list.hton[i], thd, bfn->fn,
> bfn->arg);
> - return FALSE;
> + res |= hton_list.hton[i]->binlog_func(hton_list.hton[i], thd,
> + bfn->fn, bfn->arg);
> + return res != 0;
> }
>
> int ha_reset_logs(THD *thd)
> {
> binlog_func_st bfn= {BFN_RESET_LOGS, 0};
> - binlog_func_foreach(thd,&bfn);
> - return 0;
> + return binlog_func_foreach(thd,&bfn);
> }
>
> -void ha_reset_slave(THD* thd)
> +int ha_reset_slave(THD* thd)
> {
> binlog_func_st bfn= {BFN_RESET_SLAVE, 0};
> - binlog_func_foreach(thd,&bfn);
> + return binlog_func_foreach(thd,&bfn);
> }
>
> void ha_binlog_wait(THD* thd)
> @@ -4263,15 +4264,13 @@ void ha_binlog_wait(THD* thd)
> int ha_binlog_end(THD* thd)
> {
> binlog_func_st bfn= {BFN_BINLOG_END, 0};
> - binlog_func_foreach(thd,&bfn);
> - return 0;
> + return binlog_func_foreach(thd,&bfn);
> }
>
> int ha_binlog_index_purge_file(THD *thd, const char *file)
> {
> binlog_func_st bfn= {BFN_BINLOG_PURGE_FILE, (void *)file};
> - binlog_func_foreach(thd,&bfn);
> - return 0;
> + return binlog_func_foreach(thd,&bfn);
> }
>
> struct binlog_log_query_st
>
> === modified file 'sql/handler.h'
> --- a/sql/handler.h 2011-01-26 21:12:56 +0000
> +++ b/sql/handler.h 2011-02-01 09:48:52 +0000
> @@ -2607,7 +2607,7 @@ void trans_register_ha(THD *thd, bool al
> #ifdef HAVE_NDB_BINLOG
> int ha_reset_logs(THD *thd);
> int ha_binlog_index_purge_file(THD *thd, const char *file);
> -void ha_reset_slave(THD *thd);
> +int ha_reset_slave(THD *thd);
> void ha_binlog_log_query(THD *thd, handlerton *db_type,
> enum_binlog_command binlog_command,
> const char *query, uint query_length,
> @@ -2615,12 +2615,13 @@ void ha_binlog_log_query(THD *thd, handl
> void ha_binlog_wait(THD *thd);
> int ha_binlog_end(THD *thd);
> #else
> -#define ha_reset_logs(a) do {} while (0)
> -#define ha_binlog_index_purge_file(a,b) do {} while (0)
> -#define ha_reset_slave(a) do {} while (0)
> +static inline int ha_int_dummy(void) { return 0; }
> +#define ha_reset_logs(a) ha_int_dummy()
> +#define ha_binlog_index_purge_file(a,b) ha_int_dummy()
> +#define ha_reset_slave(a) ha_int_dummy()
> #define ha_binlog_log_query(a,b,c,d,e,f,g) do {} while (0)
> #define ha_binlog_wait(a) do {} while (0)
> -#define ha_binlog_end(a) do {} while (0)
> +#define ha_binlog_end(a) ha_int_dummy()
> #endif
>
> const char *get_canonical_filename(handler *file, const char *path,
>
> === modified file 'sql/rpl_slave.cc'
> --- a/sql/rpl_slave.cc 2011-01-24 03:58:22 +0000
> +++ b/sql/rpl_slave.cc 2011-02-01 09:48:52 +0000
> @@ -5789,7 +5789,16 @@ int reset_slave(THD *thd, Master_info* m
> goto err;
> }
>
> - ha_reset_slave(thd);
> + if ((error= ha_reset_slave(thd)))
> + {
> + /*
> + Failed to reset slave in handler(which should have pushed warnings
> + describing the problem in more detail) -> use the unknown error
> + code already set
> + */
> + errmsg= "ha_reset_slave failed";
> + goto err;
> + }
>
> // delete relay logs, clear relay log coordinates
> if ((error= mi->rli->purge_relay_logs(thd,
>
>
>
>
>
