Re: [PATCH] Replace / with : in ConnectionString

[Nikolay Sivov]

On 3/31/22 21:05, Florian Manschwetus wrote:
> From: manschwetusCS
>
> In Windows ODBC it seems to be legal to use "host/port", which is not legal with Linux ODBC.
> This patch aims to find and replace such in the ConnectionString

Hi, Florian.

Thanks for the patch. I have a few comments.
> @@ -1779,10 +1779,21 @@ SQLRETURN WINAPI SQLDriverConnect(SQLHDBC hdbc, SQLHWND hwnd, SQLCHAR *Connectio
>                                     SQLCHAR *conn_str_out, SQLSMALLINT conn_str_out_max,
>                                     SQLSMALLINT *ptr_conn_str_out, SQLUSMALLINT driver_completion)
>   {
> -
>
> static const char *serverStr="SERVER=";
>  struct SQLDriverConnect_params params = { hdbc, hwnd, ConnectionString, Length, conn_str_out,
>                                            conn_str_out_max, ptr_conn_str_out, driver_completion };
>  SQLRETURN ret;
> -
>
> SQLCHAR *serverPos;
> -
>
> for (serverPos = strstr(ConnectionString,serverStr);(*serverPos != ';') && (*serverPos != '\0'); ++serverPos)
> -
>
> {
> -
>    if (*serverPos == '/')
> -
>    {
> -
>       *serverPos = ':';
> -
>       break;
> -
>    }
> -
>
> }
> -

My understanding is that connection string attributes have to be passed 
as is to underlying driver/unixodbc proxy. If server port syntax differs 
between implementations, does that mean different drivers have different 
syntax? If that's the case I don't think you can swap one for another 
like that. For example I see some examples for SQL Server that are using 
"host,port" syntax instead.

So this needs to be investigated across drivers on Windows and unixodbc 
to see what needs to happen exactly. Depending on syntax requirements, 
it's possible we need to change it only for some drivers, or maybe 
drivers need to be fixed.

If it turns out we do need to modify connection string, cleaner way is 
to tokenize it fully first, then tweak parts that need tweaking, and 
assemble it back. In your fix you're changing original connection 
string, which sounds incorrect. Another thing is that attribute names 
are case insensitive afaik, so comparison you're using is too strict.