List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:November 2 2010 11:33am
Subject:Re: bzr commit into mysql-5.5-bugteam branch (mats.kindahl:3258)
Bug#57108
View as plain text  
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);
>
>
>
>
>

Thread
bzr commit into mysql-5.5-bugteam branch (mats.kindahl:3258) Bug#57108Mats Kindahl25 Oct
Re: bzr commit into mysql-5.5-bugteam branch (mats.kindahl:3258)Bug#57108Alfranio Correia2 Nov
  • Re: bzr commit into mysql-5.5-bugteam branch (mats.kindahl:3258)Bug#57108Mats Kindahl2 Nov
    • Re: bzr commit into mysql-5.5-bugteam branch (mats.kindahl:3258)Bug#57108Alfranio Correia3 Nov
      • Re: bzr commit into mysql-5.5-bugteam branch (mats.kindahl:3258)Bug#57108Mats Kindahl3 Nov
Re: bzr commit into mysql-5.5-bugteam branch (mats.kindahl:3258)Bug#57108He Zhenxing4 Nov