Hi Sachin, The overall patch looks ok. I, however, have a few minor comments inline.
On Thu, Jan 19, 2017 at 1:21 AM, SachinSetiya <[email protected]> wrote: > revision-id: 98b2a9c967a5eaa1f99bb3ef229ff2af62018ffe > (mariadb-10.0.28-34-g98b2a9c) > parent(s): 9bf92706d19761722b46d66a671734466cb6e98e > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2017-01-19 11:50:59 +0530 > message: > > MDEV-4774 Strangeness with max_binlog_stmt_cache_size Settings > > Problem:- When setting max_binlog_stmt_cache_size=18446744073709547520 > from either command line or .cnf file, server fails to start. > > Solution:- Added one more function eval_num_suffix_ull , which uses > strtoull to get unsigned ulonglong from string. And getopt_ull calles this > Typo s/calles/calls/ > function instead of eval_num_suffix. Also changed previous eval_num_suffix > to > eval_num_suffix_ll to remain consistent. > > --- > .../r/binlog_max_binlog_stmt_cache_size.result | 14 +++++ > .../binlog/t/binlog_max_binlog_stmt_cache_size.opt | 1 + > .../t/binlog_max_binlog_stmt_cache_size.test | 11 ++++ > mysys/my_getopt.c | 66 > ++++++++++++++++++---- > 4 files changed, 81 insertions(+), 11 deletions(-) > > diff --git > a/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result > b/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result > new file mode 100644 > index 0000000..6fbec90 > --- /dev/null > +++ b/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result > @@ -0,0 +1,14 @@ > +select @@max_binlog_stmt_cache_size; > +@@max_binlog_stmt_cache_size > +18446744073709547520 > +set global max_binlog_stmt_cache_size= 18446744073709547520; > +select @@max_binlog_stmt_cache_size; > +@@max_binlog_stmt_cache_size > +18446744073709547520 > +set global max_binlog_stmt_cache_size= 18446744073709547519; > +Warnings: > +Warning 1292 Truncated incorrect max_binlog_stmt_cache_size > value: '18446744073709547519' > +select @@max_binlog_stmt_cache_size; > +@@max_binlog_stmt_cache_size > +18446744073709543424 > +set global max_binlog_stmt_cache_size= 18446744073709547520; > diff --git a/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt > b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt > new file mode 100644 > index 0000000..c52ef14 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt > @@ -0,0 +1 @@ > +--max_binlog_stmt_cache_size=18446744073709547520 > diff --git a/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test > b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test > new file mode 100644 > index 0000000..bc30b48 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test > @@ -0,0 +1,11 @@ > +source include/have_log_bin.inc; > +select @@max_binlog_stmt_cache_size; > + > +--let $cache_size=`select @@max_binlog_stmt_cache_size;` > + > +set global max_binlog_stmt_cache_size= 18446744073709547520; > +select @@max_binlog_stmt_cache_size; > + > +set global max_binlog_stmt_cache_size= 18446744073709547519; > +select @@max_binlog_stmt_cache_size; > I would also add tests for ULLONG_MAX and ULLONG_MAX +/- 1. +--eval set global max_binlog_stmt_cache_size= $cache_size > diff --git a/mysys/my_getopt.c b/mysys/my_getopt.c > index 2a45571..0934a50 100644 > --- a/mysys/my_getopt.c > +++ b/mysys/my_getopt.c > @@ -895,11 +895,32 @@ my_bool getopt_compare_strings(register const char > *s, register const char *t, > /* > function: eval_num_suffix > > + Transforms suffix like k/m/g to there real value. > Typo : s/there/their/ > +*/ > +static inline long eval_num_suffix(char *suffix, int *error) > +{ > + long num= 1; > + if (*suffix == 'k' || *suffix == 'K') > + num*= 1024L; > + else if (*suffix == 'm' || *suffix == 'M') > + num*= 1024L * 1024L; > + else if (*suffix == 'g' || *suffix == 'G') > + num*= 1024L * 1024L * 1024L; > + else if (*suffix) > + { > + *error= 1; > + return 0; > + } > + return num; > +} > Please add a new line here (as a separator). > +/* > + function: eval_num_suffix_ll > + > Transforms a number with a suffix to real number. Suffix can > be k|K for kilo, m|M for mega or g|G for giga. > */ > > -static longlong eval_num_suffix(char *argument, int *error, char > *option_name) > +static longlong eval_num_suffix_ll(char *argument, int *error, char > *option_name) > { > char *endchar; > longlong num; > DBUG_ENTER("eval_num_suffix"); You should also update the function name. > @@ -916,23 +937,46 @@ static longlong eval_num_suffix(char *argument, int > *error, char *option_name) > *error= 1; > DBUG_RETURN(0); > } > - if (*endchar == 'k' || *endchar == 'K') > - num*= 1024L; > - else if (*endchar == 'm' || *endchar == 'M') > - num*= 1024L * 1024L; > - else if (*endchar == 'g' || *endchar == 'G') > - num*= 1024L * 1024L * 1024L; > - else if (*endchar) > - { > + num*= eval_num_suffix(endchar, error); > + if (*error) > fprintf(stderr, > "Unknown suffix '%c' used for variable '%s' (value '%s')\n", > *endchar, option_name, argument); > + DBUG_RETURN(num); > +} > + > +/* > + function: eval_num_suffix_ull > + > + Transforms a number with a suffix to positive Integer. Suffix can > + be k|K for kilo, m|M for mega or g|G for giga. > +*/ > + > +static ulonglong eval_num_suffix_ull(char *argument, int *error, char > *option_name) > The above line >80 chars, please break it into 2 lines. > +{ > + char *endchar; > + ulonglong num; > + DBUG_ENTER("eval_num_suffix"); > Update the function name. > + > + *error= 0; > + errno= 0; > + num= strtoull(argument, &endchar, 10); > + if (errno == ERANGE) > + { > + my_getopt_error_reporter(ERROR_LEVEL, > + "Incorrect integer value: '%s'", argument); > *error= 1; > DBUG_RETURN(0); > } > + num*= eval_num_suffix(endchar, error); > + if (*error) > + fprintf(stderr, > + "Unknown suffix '%c' used for variable '%s' (value '%s')\n", > + *endchar, option_name, argument); > DBUG_RETURN(num); > } > > + > /* > function: getopt_ll > > @@ -946,7 +990,7 @@ static longlong eval_num_suffix(char *argument, int > *error, char *option_name) > > static longlong getopt_ll(char *arg, const struct my_option *optp, int > *err) > { > - longlong num=eval_num_suffix(arg, err, (char*) optp->name); > + longlong num=eval_num_suffix_ll(arg, err, (char*) optp->name); > return getopt_ll_limit_value(num, optp, NULL); > } > > @@ -1023,7 +1067,7 @@ longlong getopt_ll_limit_value(longlong num, const > struct my_option *optp, > > static ulonglong getopt_ull(char *arg, const struct my_option *optp, int > *err) > { > - ulonglong num= eval_num_suffix(arg, err, (char*) optp->name); > + ulonglong num= eval_num_suffix_ull(arg, err, (char*) optp->name); > return getopt_ull_limit_value(num, optp, NULL); > } > > Best, Nirbhay
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
