List:Internals« Previous MessageNext Message »
From:Arjen Lentz Date:April 9 2009 10:31am
Subject:Re: your patch for BUG#40368 "mysqld_safe to honour underscore same as dash on server options "
View as plain text  
Hi Guilhem

On 02/04/2009, at 10:57 PM, Guilhem Bichot wrote:
> I'm looking at your patch; agree with the bug's description and idea  
> of the patch.
> Good that you insisted on it, we can be slow sometimes :)

I'm happy that you picked it up.


> The patch is:
>
> --- mysql-5.0.67.orig/scripts/mysqld_safe.sh    2008-08-04  
> 13:20:02.000000000 +0100
> +++ mysql-5.0.67/scripts/mysqld_safe.sh 2008-10-28  
> 22:11:06.000000000 +0000
> @@ -59,6 +59,14 @@
>   fi
>
>   for arg do
> +
> +    # Replaces all underscores with dashes (excluding the two  
> initial ones
> +    # and any instances after the first = character) since mysqld  
> accepts
> +    # variables in this format.
> +    substr_a=`echo $arg | sed -e 's/\(...\)\(.*\)=\(.*\)/\2/'`
>
> Here, you eliminate the 3 first characters ("..."). So if arg is -- 
> open_files_limit=32, substr_a is "pen_files_limit". In the end it  
> works, but why not eliminate only two (two dots)? Why use ".." and  
> not "--" (What else can there be aprt from "--", in this position?).
>
> For the unlikely case where the user would do (gasp):
> --datadir=/some/path/with_=_in/it ,
> we wouldn't want to change "_" to "-" in the path. One would have to  
> be crazy, but I prefer to avoid introducing a rare bug when fixing  
> another.
> I propose two precautions to avoid that:
> - forcing the regex to match at start of string, only
> - forcing it to not be greedy by picking the first "="(see below):
>
> +    substr_b=`echo $substr_a | sed s/_/-/g`
> +    arg=`echo $arg | sed s/$substr_a/$substr_b/g`
>
> With the --datadir option value above, the substituation here fails  
> because the sed command line expands to:
> sed s/atadir=/some/path/with_/atadir=/some/path/with-/g
> (at least with "bash" on my Linux laptop).
> I am thinking about this:
> substr_a=`echo $arg | sed -e 's/^\(..\)\([^=]*\)=\(.*\)/\2/'`
> though I haven't tested on <obscure platform>.
>
> Any ideas?

Sounds sensible.


> You mentioned fixing my_print_defaults, why is it needed?

It was another approach to resolving the same issue, if I remember  
correctly.


> By the way - paperwork. You have already signed the CLA in 2007; you  
> will be happy to know that there is more to do (somehow contribution  
> agreements have been revised since you signed):
> http://forge.mysql.com/wiki/Contributing_Code#Paperwork

Well, two things:
  - Erik Ljungstrom did the patch, not I (he's cc'd on this email).
  - This is a few lines of script/code, I think you are quite fine  
just using it. Particularly since you just rewrote it! From past  
experience at MySQL, we never insisted on the CLA for very small  
patches.


Regards,
Arjen.
-- 
Arjen Lentz, Director @ Open Query (http://openquery.com)
Affordable Training and ProActive Support for MySQL & related  
technologies

My blog is at http://arjen-lentz.livejournal.com
OurDelta: free enhanced builds for MySQL @ http://ourdelta.org

Thread
your patch for BUG#40368"mysqld_safe to honour underscore same as dash on server options "Guilhem Bichot2 Apr
  • Re: your patch for BUG#40368 "mysqld_safe to honour underscore same as dash on server options "Arjen Lentz9 Apr
    • Re: your patch for BUG#40368 "mysqld_safe to honour underscore same as dash on server options "Masood Mortazavi17 Apr