Hi Mats,
Great work,
Please, find in what follows some requests/suggestions to improve the patch.
STATUS
------
Patch conditionally approved.
REQUESTS
---------
1. Please, ask bjorn to check the changes to ysql-test/mysql-test-run.pl
2. Why do you need
static char my_defaults_extra_file_buffer[FN_REFLEN];
static char my_defaults_file_buffer[FN_REFLEN]; ?
It would be better to have
+static int
+fn_expand(const char *filename, const char **filename_var)
+{
+ char dir[FN_REFLEN];
+ char buf[FN_REFLEN];
3. Are the different return values used anywhere?
I don't think this is used anywhere:
+ RETURNS
+ 0 All OK
+ 2 Out of memory or path to long
+ 3 Not able to get working directory
+ */
my_search_option_files -> fn_expand -> my_load_defaults/load_defaults
Besides, there are places where the return value is not verified.
4. Please, use DBUG_RETURN instead of return.
+ if (forced_extra_defaults&& !defaults_already_read)
+ {
+ int error= fn_expand(forced_extra_defaults, my_defaults_extra_file_buffer,
+&my_defaults_extra_file);
+ if (error)
+ return error;
+ }
5. Please, s/was/is/
+/*
+ Expand a file name so that the current working directory is added if
+ the name was relative.
+*/
6. Please, s/int/my_bool/
+static int defaults_already_read= 0;
Cheers.
> === modified file 'mysys/default.c'
> --- a/mysys/default.c 2010-07-23 20:17:55 +0000
> +++ b/mysys/default.c 2010-10-25 20:03:08 +0000
> @@ -66,7 +66,11 @@
> const char *args_separator= "----args-separator----";
> const char *my_defaults_file=0;
> const char *my_defaults_group_suffix=0;
> -char *my_defaults_extra_file=0;
> +const char *my_defaults_extra_file=0;
> +
> +static int defaults_already_read= 0;
> +static char my_defaults_extra_file_buffer[FN_REFLEN];
> +static char my_defaults_file_buffer[FN_REFLEN];
>
> /* Which directories are searched for options (and in which order) */
>
> @@ -140,6 +144,36 @@ static char *remove_end_comment(char *pt
>
>
> /*
> + Expand a file name so that the current working directory is added if
> + the name was relative.
> +
> + RETURNS
> + 0 All OK
> + 2 Out of memory or path to long
> + 3 Not able to get working directory
> + */
> +
> +static int
> +fn_expand(const char *filename, char buf[FN_REFLEN], const char **filename_var)
> +{
> + char dir[FN_REFLEN];
> + const int flags= MY_UNPACK_FILENAME | MY_SAFE_PATH | MY_RELATIVE_PATH;
> + const char *result_path= NULL;
> + DBUG_ENTER("fn_expand");
> + DBUG_PRINT("enter", ("filename: %s, buf: 0x%lx", filename, (unsigned long) buf));
> + if (my_getwd(dir, sizeof(dir), MYF(0)))
> + DBUG_RETURN(3);
> + DBUG_PRINT("debug", ("dir: %s", dir));
> + if (fn_format(buf, filename, dir, NULL, flags) == NULL ||
> + (result_path= my_strdup(buf, MYF(0))) == NULL)
> + DBUG_RETURN(2);
> + DBUG_PRINT("return", ("result: %s", result_path));
> + DBUG_ASSERT(result_path != NULL);
> + *filename_var= result_path;
> + DBUG_RETURN(0);
> +}
> +
> +/*
> Process config files in default directories.
>
> SYNOPSIS
> @@ -167,6 +201,7 @@ static char *remove_end_comment(char *pt
> 0 ok
> 1 given cinf_file doesn't exist
> 2 out of memory
> + 3 Can't get current working directory
>
> The global variable 'my_defaults_group_suffix' is updated with value for
> --defaults_group_suffix
> @@ -189,11 +224,23 @@ int my_search_option_files(const char *c
> if (! my_defaults_group_suffix)
> my_defaults_group_suffix= getenv(STRINGIFY_ARG(DEFAULT_GROUP_SUFFIX_ENV));
>
> - if (forced_extra_defaults)
> - my_defaults_extra_file= (char *) forced_extra_defaults;
> -
> - if (forced_default_file)
> - my_defaults_file= forced_default_file;
> + if (forced_extra_defaults&& !defaults_already_read)
> + {
> + int error= fn_expand(forced_extra_defaults, my_defaults_extra_file_buffer,
> +&my_defaults_extra_file);
> + if (error)
> + return error;
> + }
> +
> + if (forced_default_file&& !defaults_already_read)
> + {
> + int error= fn_expand(forced_default_file, my_defaults_file_buffer,
> +&my_defaults_file);
> + if (error)
> + return error;
> + }
> +
> + defaults_already_read= 1;
>
> /*
> We can only handle 'defaults-group-suffix' if we are called from
> @@ -236,15 +283,15 @@ int my_search_option_files(const char *c
> group->type_names[group->count]= 0;
> }
>
> - if (forced_default_file)
> + if (my_defaults_file)
> {
> if ((error= search_default_file_with_ext(func, func_ctx, "", "",
> - forced_default_file, 0))< 0)
> + my_defaults_file, 0))< 0)
> goto err;
> if (error> 0)
> {
> fprintf(stderr, "Could not open required defaults file: %s\n",
> - forced_default_file);
> + my_defaults_file);
> goto err;
> }
> }
>
> === modified file 'sql/sql_plugin.cc'
> --- a/sql/sql_plugin.cc 2010-10-07 15:52:34 +0000
> +++ b/sql/sql_plugin.cc 2010-10-25 20:03:08 +0000
> @@ -1738,7 +1738,11 @@ bool mysql_install_plugin(THD *thd, cons
> mysql_mutex_lock(&LOCK_plugin);
> mysql_rwlock_wrlock(&LOCK_system_variables_hash);
>
> - my_load_defaults(MYSQL_CONFIG_NAME, load_default_groups,&argc,&argv,
> NULL);
> + if (my_load_defaults(MYSQL_CONFIG_NAME, load_default_groups,&argc,&argv,
> NULL))
> + {
> + report_error(REPORT_TO_USER, ER_PLUGIN_IS_NOT_LOADED, name->str);
> + goto err;
> + }
> error= plugin_add(thd->mem_root, name, dl,&argc, argv, REPORT_TO_USER);
> if (argv)
> free_defaults(argv);
>
>
>
>
>
