Hi!
On Nov 26, Tatjana A Nuernberg wrote:
> ChangeSet@stripped, 2007-11-26 06:21:48+01:00, tnurnberg@stripped +17 -0
> Bug#31177: Server variables can't be set to their current values
>
> Default values of variables were not subject to upper/lower bounds
> and step, while setting variables was. Bounds and step are also
> applied to defaults now; defaults are corrected quietly, values
> given by the user are corrected, and a correction-warning is thrown
> as needed. Lastly, very large values could wrap around, starting
> from 0 again. They are bounded at the maximum value for the
> respective data-type now if no lower maximum is specified in the
> variable's definition.
>
> diff -Nrup a/mysys/my_getopt.c b/mysys/my_getopt.c
> --- a/mysys/my_getopt.c 2007-10-04 10:33:56 +02:00
> +++ b/mysys/my_getopt.c 2007-11-26 06:21:46 +01:00
> @@ -59,6 +61,13 @@ char *disabled_my_option= (char*) "0";
>
> my_bool my_getopt_print_errors= 1;
>
> +/*
> + This is a flag that can be set in client programs. 1 means that
> + my_getopt will skip over options it does not know how to handle.
> +*/
> +
> +my_bool my_getopt_skip_unknown= 0;
what ? did you simply copy *everything* from 5.1 ? this part of
my_getopt.c of 5.1 is not necessary for this bugfix.
> static void default_reporter(enum loglevel level,
> const char *format, ...)
> {
> @@ -73,6 +82,11 @@ static void default_reporter(enum loglev
> fflush(stderr);
> }
>
> +static void null_reporter(enum loglevel level,
> + const char *format, ...)
> +{
> +}
I don't like this null_reporter idea. Let init_one_value issue warnings
normally for every value adjustment.
Of course there should be no warnings, that is, you need to fix all
options where default values are outside of min..max range.
> /*
> function: handle_options
>
> @@ -105,12 +119,17 @@ int handle_options(int *argc, char ***ar
> int error;
>
> LINT_INIT(opt_found);
> +
> + /* handle_options() assumes arg0 (program name) always exists */
> + DBUG_ASSERT(argc && *argc >= 1);
> + DBUG_ASSERT(argv && *argv);
> (*argc)--; /* Skip the program name */
> (*argv)++; /* --- || ---- */
> init_variables(longopts);
>
> for (pos= *argv, pos_end=pos+ *argc; pos != pos_end ; pos++)
> {
> + char **first= pos;
> char *cur_arg= *pos;
> if (cur_arg[0] == '-' && cur_arg[1] && !end_of_options) /* must
> be opt */
> {
> @@ -217,12 +236,11 @@ int handle_options(int *argc, char ***ar
> /*
> We were called with a special prefix, we can reuse opt_found
> */
> - opt_str+= (special_opt_prefix_lengths[i] + 1);
> + opt_str+= special_opt_prefix_lengths[i] + 1;
> + length-= special_opt_prefix_lengths[i] + 1;
> if (i == OPT_LOOSE)
> option_is_loose= 1;
> - if ((opt_found= findopt(opt_str, length -
> - (special_opt_prefix_lengths[i] + 1),
> - &optp, &prev_found)))
> + if ((opt_found= findopt(opt_str, length, &optp, &prev_found)))
this is also not required for this bugfix. although it fixes another
bug, so it's ok. But mention it in the delta comment.
> {
> if (opt_found > 1)
> {
> @@ -255,11 +273,25 @@ int handle_options(int *argc, char ***ar
> }
> break; /* break from the inner loop, main loop continues */
> }
> + i= -1; /* restart the loop */
same here
> }
> }
> }
> if (!opt_found)
> {
> + if (my_getopt_skip_unknown)
> + {
> + /*
> + preserve all the components of this unknown option, this may
> + occurr when the user provides options like: "-O foo" or
> + "--set-variable foo" (note that theres a space in there)
> + Generally, these kind of options are to be avoided
> + */
> + do {
> + (*argv)[argvpos++]= *first++;
> + } while (first <= pos);
> + continue;
> + }
> if (must_be_var)
> {
> if (my_getopt_print_errors)
> @@ -733,23 +769,40 @@ static longlong eval_num_suffix (char *a
>
> static longlong getopt_ll(char *arg, const struct my_option *optp, int *err)
> {
> - longlong num;
> + longlong num=eval_num_suffix(arg, err, (char*) optp->name);
> + return getopt_ll_limit_value(num, optp);
> +}
> +
> +/*
> + function: getopt_ll_limit_value
> +
> + Applies min/max/block_size to a numeric value of an option.
> + Returns "fixed" value.
> +*/
> +
> +static longlong getopt_ll_limit_value(longlong num,
> + const struct my_option *optp)
> +{
> + longlong old= num;
> + char buf1[255] __attribute__((unused)), buf2[255] __attribute__((unused));
> ulonglong block_size= (optp->block_size ? (ulonglong) optp->block_size :
> 1L);
> -
> - num= eval_num_suffix(arg, err, (char*) optp->name);
> +
> if (num > 0 && (ulonglong) num > (ulonglong) optp->max_value
> &&
> optp->max_value) /* if max value is not set -> no upper limit */
> - {
> - char buf[22];
> - my_getopt_error_reporter(WARNING_LEVEL,
> - "Truncated incorrect %s value: '%s'",
> - optp->name, llstr(num, buf));
> -
> num= (ulonglong) optp->max_value;
> - }
> +
> num= ((num - optp->sub_size) / block_size);
> num= (longlong) (num * block_size);
> - return max(num, optp->min_value);
> +
> + num= max(num, optp->min_value);
> +
> + if (num != old)
> + my_getopt_error_reporter(WARNING_LEVEL,
> + "option '%s' (type %d): "
this '(type %d)'. either make it a user-readable type name (a string), or
remove altogether.
> + "signed value '%s' adjusted to '%s'",
> + optp->name, optp->var_type & GET_TYPE_MASK,
> + llstr(old, buf1), llstr(num, buf2));
> + return num;
> }
>
> /*
> @@ -780,6 +848,13 @@ ulonglong getopt_ull_limit_value(ulonglo
> }
> if (num < (ulonglong) optp->min_value)
> num= (ulonglong) optp->min_value;
> +
> + if (num != old)
> + my_getopt_error_reporter(WARNING_LEVEL,
> + "option '%s' (type %d): "
same here
> + "unsigned value '%s' adjusted to '%s'",
> + optp->name, optp->var_type & GET_TYPE_MASK,
> + ullstr(old, buf1), ullstr(num, buf2));
> return num;
> }
>
> diff -Nrup a/sql/set_var.cc b/sql/set_var.cc
> --- a/sql/set_var.cc 2007-10-29 08:25:38 +01:00
> +++ b/sql/set_var.cc 2007-11-26 06:21:46 +01:00
> @@ -1448,6 +1453,44 @@ static void fix_server_id(THD *thd, enum
> }
>
> +static ulonglong fix_unsigned(ulonglong in,
> + const struct my_option *option_limits,
> + ulonglong *chk)
> +{
> + ulonglong out= getopt_ull_limit_value(in, option_limits);
> + /*
> + If we've already truncated or there is no step,
> + the check is immaterial.
yeah, null and void
> + */
> + if (chk && (option_limits->block_size > 1) && (*chk == in))
> + {
> + struct my_option tmp_limits;
> + memcpy(&tmp_limits, option_limits, sizeof(struct my_option));
> + tmp_limits.block_size= 1;
why ???
> + if (getopt_ull_limit_value(in, &tmp_limits) == in)
> + *chk= out;
> + }
> + return out;
> +}
> +
> +static void throw_bounds_warning(THD *thd, const char *name,
> + ulonglong from, ulonglong to)
> +{
> + if (from != to)
> + {
> + char buf[44], *p;
> + ullstr(from, buf);
> + p= strchr(buf, '\0');
> + strcat(p, "->");
> + ullstr(to, p+2);
> + push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> + ER_TRUNCATED_WRONG_VALUE,
> + ER(ER_TRUNCATED_WRONG_VALUE), name,
> + buf);
no, the message is
Truncated incorrect %-.32s value: '%-.128s'"
and the second parameter is the *value*. It's not "oldvalue->newvalue".
Just print the old value, like we do everywhere else, we should be
consistent.
> + }
> +}
> +
> +
> sys_var_long_ptr::
> sys_var_long_ptr(const char *name_arg, ulong *value_ptr_arg,
> sys_after_update_func after_update_arg)
> @@ -1468,9 +1511,20 @@ bool sys_var_long_ptr_global::update(THD
> ulonglong tmp= var->save_result.ulonglong_value;
> pthread_mutex_lock(guard);
> if (option_limits)
> - *value= (ulong) getopt_ull_limit_value(tmp, option_limits);
> + *value= (ulong) fix_unsigned(tmp, option_limits,
> + &var->save_result.ulonglong_value);
> else
> + {
> +#if SIZEOF_LONG < SIZEOF_LONG_LONG
> + /* Avoid overflows on 32 bit systems */
> + if (tmp > (ulonglong) ~(ulong) 0)
> + tmp= ((ulonglong) ~(ulong) 0);
> +#endif
> *value= (ulong) tmp;
> + }
> +
> + throw_bounds_warning(thd, name, var->save_result.ulonglong_value, *value);
I don't think you're supposed to do any checks or issue any warnings in
::update(), they belong to ::check().
> pthread_mutex_unlock(guard);
> return 0;
> }
Regards / Mit vielen Grüssen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Developer
/_/ /_/\_, /___/\___\_\___/ MySQL GmbH, Dachauer Str. 37, D-80335 München
<___/ Geschäftsführer: Kaj Arnö - HRB
München 162140
