Update SQLCancel.c

[matthew-wozniczka]

Refactored how SQLCancel checks for the driver returning 01S05 to be a bit easier to read, and a tiny bit faster (Can skip a call to SQLGetDiagField/SQLGetDiagFieldW)

[lurcher]

On 08/03/2024 23:55, Matthew Wozniczka wrote: Refactored how |SQLCancel| checks for the driver returning 01S05 to be a bit easier to read, and a tiny bit faster (Can skip a call to |SQLGetDiagField|/|SQLGetDiagFieldW|) ------------------------------------------------------------------------ You can view, comment on, or merge this pull request online at: #161 Commit Summary * 277d1ff <277d1ff> Update SQLCancel.c File Changes (1 file <https://github.com/lurcher/unixODBC/pull/161/files>) * *M* DriverManager/SQLCancel.c <https://github.com/lurcher/unixODBC/pull/161/files#diff-85e8ac9f2eccecba2264d014735d6b7b6ca2d926b7a5802cc1de89e176772533> (47) Patch Links: * https://github.com/lurcher/unixODBC/pull/161.patch * https://github.com/lurcher/unixODBC/pull/161.diff — Reply to this email directly, view it on GitHub <#161>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYK62KYNHSUFZKHEPFBK5LYXJFX7AVCNFSM6AAAAABENTEES6VHI2DSMVQWIX3LMV43ASLTON2WKOZSGE3TMOJTGYYDEMA>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

That all looks perfectly good. But its got me looking at memcmp(state, "0\0001\000S\0000\0005\0", 10) And thinking that is making a assumption about little endian byte ordering. I know you have just taken whats there, but after accepting your change, I think I will check if that doesn't need changing.

[matthew-wozniczka]

Yeah actually I did notice that and think that might be a problem, but didn't wanna get into it, then forgot about even reporting it.

[matthew-wozniczka]

Actually... I was only looking at this file because I'm currently trying to debug a weird hang in our tests which seems to occur within the loop I modified in this pull request.

And it's on AIX, using PowerPC, a big-endian processor! And the tests which are hanging ARE testing the case where the connection has been configured as an ODBC 2.X connection, and SQLCancel is causing the cursor to be closed.

But I don't see how messing up the check like this can cause some sort of infinite loop, so I dismissed that as the cause.

[matthew-wozniczka]

One other thing I just noticed while staring at the loop is that it's using a SQLCHAR buffer, but passing it as SQLWCHAR* for unicode drivers.

This is actually unsafe, as there can be alignment issues, which can cause bus error signals, or at least lower performance (sometimes greatly)

It's safer to use a SQLWCHAR buffer, since that will surely be aligned for SQLCHAR (which I'm pretty sure is guaranteed to be char, and thus not need any special alignment)

[matthew-wozniczka]

Also, doesn't unixODBC support being compiled w/ SQLWCHAR == UTF-32? This code assumes UTF-16

[matthew-wozniczka]

unixODBC already has code to translate from SQLGetDiagFieldW -> SQLGetDiagField, so maybe it would be simpler to simply reuse it here and always compare against "01S05"?

[lurcher]

On 11/03/2024 07:12, Matthew Wozniczka wrote: Also, doesn't unixODBC support being compiled w/ |SQLWCHAR| == |UTF-32|? This code assumes |UTF-16| — Reply to this email directly, view it on GitHub <#161 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYK62JDLUZ6ZWSOT74UJZDYXVRO7AVCNFSM6AAAAABENTEES6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXG42TSMRWGM>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

Good point about the alignment, I have just changed it to use your suggestion, re allignment. AFAIK, the buffer that's being compared is already being set in a way that should work for both 2 and 4 byte SQLWCHAR, as should the rest of the code.

[lurcher]

On 11/03/2024 07:15, Matthew Wozniczka wrote: unixODBC already /has/ code to translate from |SQLGetDiagFieldW| -> |SQLGetDiagField|, so maybe it would be simpler to simply reuse it here and always compare against |"01S05"| directly? — Reply to this email directly, view it on GitHub <#161 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYK62MGFQZVBA3AQXSJFYLYXVRYZAVCNFSM6AAAAABENTEES6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXG43DEMZQGA>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

Maybe, but lets see if your search for the problem you are having unearths anything before changing something for reason of simplicity. I know its a old age thing, but as long as the "it works" part of "if it works, don't try and fix it" I like to leave well alone.

[matthew-wozniczka]

ah, I think I may know why I have a 'hang': When the old code was fetching the number of diagnostic records, it was using SQLULEN, but that field is documented to be SQLINTEGER.

Now, on little-endian platforms, this doesn't matter, since it initializes the variable to 0.

But on big-endian platforms, in 64-bit mode, it's incorrectly interpreting the returned value as 2^32 times MORE than what the driver actually tried to report.

That, combined with the early-exit from the loop being broken on big-endian platforms would probably result in something that I (actually our test automation) could misinterpret as a hang.

[matthew-wozniczka]

AFAIK, the buffer that's being compared is
already being set in a way that should work for both 2 and 4 byte
SQLWCHAR, as should the rest of the code.

I don't see how, as the memcmp hardcodes the comparison length to 10?

[lurcher]

On 11/03/2024 09:11, Matthew Wozniczka wrote: AFAIK, the buffer that's being compared is already being set in a way that should work for both 2 and 4 byte SQLWCHAR, as should the rest of the code. I don't see how, as the |memcmp| hardcodes the comparison length to 10? — Reply to this email directly, view it on GitHub <#161 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYK62ONPZGFVSQLNEZ6JCTYXV7MRAVCNFSM6AAAAABENTEES6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXHEZTGMRRGQ>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

Good spot on the original problem. But to answer the question, unless I am wrong static SQLWCHAR str_01S05[ 5 ] = { '0', '1', 'S', '0', '5' }; Will either allocate 10 or 20 bytes depending on the size of a SQLWCHAR SQLWCHAR state[6] Will allocate space for 12 or 24 bytes of something and memcmp(state, str_01S05, sizeof( str_01S05 )) Will compare however many bytes the SQLWCHAR was allocated as.

[lurcher]

On 11/03/2024 09:15, Nick Gorham wrote: On 11/03/2024 09:11, Matthew Wozniczka wrote: > > AFAIK, the buffer that's being compared is > already being set in a way that should work for both 2 and 4 byte > SQLWCHAR, as should the rest of the code. > > I don't see how, as the |memcmp| hardcodes the comparison length to 10? > > — > Reply to this email directly, view it on GitHub > <#161 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABYK62ONPZGFVSQLNEZ6JCTYXV7MRAVCNFSM6AAAAABENTEES6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXHEZTGMRRGQ>. > You are receiving this because you modified the open/close > state.Message ID: ***@***.***> > Good spot on the original problem. But to answer the question, unless I am wrong static SQLWCHAR str_01S05[ 5 ] = { '0', '1', 'S', '0', '5' }; Will either allocate 10 or 20 bytes depending on the size of a SQLWCHAR SQLWCHAR state[6] Will allocate space for 12 or 24 bytes of something and memcmp(state, str_01S05, sizeof( str_01S05 )) Will compare however many bytes the SQLWCHAR was allocated as.

Just to avoid confusion, how it was, was wrong as you said, my comments relate to what's there now.