[asterisk-dev] [Code Review] Correct character encoding for MySQL connections in Realtime w/ MySQL

[Erin Spiceland]

> On 2010-10-07 14:24:05, Terry Wilson wrote:
> > /branches/1.4/apps/app_addon_sql_mysql.c, lines 285-286
> > <https://reviewboard.asterisk.org/r/964/diff/3/?file=12672#file12672line285>
> >
> >     Since these only get used if (dbcharset), we might as well just declare them there.

Coding guidelines say not to declare variables in the middle of a block of code. Still change it? 


> On 2010-10-07 14:24:05, Terry Wilson wrote:
> > /branches/1.4/apps/app_addon_sql_mysql.c, lines 294-297
> > <https://reviewboard.asterisk.org/r/964/diff/3/?file=12672#file12672line294>
> >
> >     I don't think the if/else is necessary here. First, strsep returns the original position of data (even if no delimiter is found), so if !(dbname = strsep(&data, " ")) we already have a problem as it means that a non-optional argument is missing. Also, if dbname == NULL, then by definition data == NULL, so doing dbname = stresep(&data, "\n") is pointless as it will just set dbname to NULL again.
> >     
> >     The other problem is that putting dbcharset = strsep(&data, "\n") in the else gives it the possibility of being uninitialized, which under normal circumstances would be very unsafe and would render the check if (dbcharset) useless. Luckily, because something very wrong happened because dbname is still null, we won't get to the code that actually accesses dbcharset anyway.
> >     
> >     Since it is safe to pass the address of a NULL string to strsep(), we should just be able to do:
> >     dbname = strsep(&data, " ");
> >     dbcharset = strsep(&data, "\n"); // or even just dbcharset = data since we are just saying "give me the rest of the string"

You're right, of course. Thanks!


- Erin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/964/#review2811
-----------------------------------------------------------


On 2010-10-04 11:22:55, Erin Spiceland wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/964/
> -----------------------------------------------------------
> 
> (Updated 2010-10-04 11:22:55)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> MySQL connection code used in res_config_mysql did not specify a character set, so data retrieved from the database was encoded incorrectly. I have added an optional configuration value which goes in res_mysql.conf which tells MySQL what character set to use when transmitting the data.  This bug was also present in the MYSQL dialplan application, so I have fixed it there as well.  Connection code both places now sends the specified character set (or "latin1" by default, as MySQL.com says this is the most commonly used character set) during connection and executes "SET NAME charset" immediately after connecting. Executing "SET NAMES charset" is necessary because it sets "character_set_connection" and "collation_connection" to the requested charsets instead of leaving them set to the default for the database which is being used.
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/apps/app_addon_sql_mysql.c 1132 
>   /branches/1.4/configs/res_mysql.conf.sample 1132 
>   /branches/1.4/res/res_config_mysql.c 1132 
> 
> Diff: https://reviewboard.asterisk.org/r/964/diff
> 
> 
> Testing
> -------
> 
> Tested with 'utf8', 'latin1', and '' specified with utf8 and latin1 data. I also wrote an external test to test the dialplan application. I tested with valid and invalid charset names.  MySQL connection fails when the charset is not valid or is not supported.
> 
> 
> Thanks,
> 
> Erin
> 
>