Assorted improvements from NCBI as of May 2024 #559

ucko · 2024-05-30T14:35:25Z

This PR contains seven (mostly formally) interdependent commits from #555; I've split off what I cleanly could, and in particular split the bulk.c changes from 1512645 into e6facc4 so that I can have a separate BCP-focused PR, leaving b6ab56f for here.

freddy77 · 2024-06-02T09:33:17Z

include/freetds/tds.h

-void tdsdump_log(const char* file, unsigned int level_line, const char *fmt, ...)
+void tdsdump_do_log(const char* file, unsigned int level_line,
+		    const char *fmt, ...)


No, this is an ABI and must be exported with this name

I thought tds* were generally internal; I do see that libsybdb exposes tdsdump_open, but that function's not in play here.

Yes, usually yes. BTW, it's only tdsdump_open (and tdsdbopen) that are in ABI, so tdsdump_log can be changed.

freddy77 · 2025-01-30T20:54:37Z

include/cspublic.h

-#define cs_dt_crack cs_dt_crack_v2
+#ifndef _FREETDS_LIBRARY_SOURCE
+#  undef cs_dt_crack
+#  define cs_dt_crack cs_dt_crack_v2
+#endif


I prefer the previous style. Shorter and does not require adding a new _FREETDS_LIBRARY_SOURCE definition.

Understood, but it interacts poorly with mass renaming (e.g., tacking on _ftds14 suffixes across the board) that would otherwise allow coexistence with commercial MS/SAP libraries and/or other analogously patched FreeTDS versions. I'm obviously not proposing to push any such renaming, just the changes that make it fully possible.

I don't understand the need for mass renaming. I don't think it's possible to use both proprietary library and FreeTDS without breaking ABI (Windows and ODBC excluded), the 2 libraries will clash.
Usually to allow this usually libraries use symbol prefixes instead of suffix (for instance adding ftds_ before all exported functions, I know libiconv is adding lib prefix to functions).

Yeah, this is a special case -- the C++ Toolkit performs mass renaming for the FreeTDS versions it bundles, with each version's wrapper in a separate namespace but all wrappers implementing the same base classes.

freddy77 · 2025-01-30T20:55:56Z

include/freetds/bool.h

+#include <config.h>
+


Including config.h is pretty standard in the source files, not in headers.

That's fair. I would like for it to land before any declarations, but another look appears to indicate that adjusting src/ctlib/unittest/common.h to include config.h ahead of ctpublic.h should suffice at this point.

freddy77 · 2025-01-30T20:59:02Z

include/freetds/odbc.h

-	SQLSMALLINT sql_desc_count;
+	SQLUSMALLINT sql_desc_count;


In this structure the types matches the relative type in descriptors, in this case SQL_DESC_COUNT from SQLSetDescField.

Good point. As it stands, the type doesn't match desc_alloc_records's count argument, but I suppose that can and should change instead.

freddy77 · 2025-01-30T21:10:16Z

src/tds/data.c

@@ -723,6 +722,22 @@ tds_generic_get(TDSSOCKET * tds, TDSCOLUMN * curcol)
 	tdsdump_log(TDS_DBG_INFO1, "tds_get_data: type %d, varint size %d\n", curcol->column_type, curcol->column_varint_size);
 	switch (curcol->column_varint_size) {
 	case 4:
+		if (!is_blob_type(curcol->column_type)) {
+			/* Any other non-BLOB type (e.g., XSYBCHAR) */


Varint for XSYBCHAR is 2, not 4.
Is not this if handling also below case (SYBLONGBINARY) ?
To be honest I would invert varint 4 and 5, meaning "old" blobs (SYBTEXT, SYBNTEXT, SYBIMAGE) having a length for pointer (1 byte) and one for data (4 bytes) marked with 5 (1+4) and "new" blobs (SYBLONGBINARY and similar) with 4.

Oops, yeah, in retrospect I should have called out SYBLONG{BINARY,CHAR} here, and the SYBLONGBINARY code was indeed unused.

src/tds/ncbi_strerror.c

src/tds/sspi.c

src/tds/util.c

This distinction will initially be useful only for ctlib and dblib, but applying it across the board may avoid possible confusion if it turns out to be useful elsewhere too. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

* Account for possible renaming of cs_dt_crack (not just aliasing it to ..._v2), dbopen, tdsdump_dump_buf, and tdsdump_log; for the latter two, change the underlying functions' names to tdsdump_do_... modulo possible mass renaming. * ctlib/unittests/common.h: Include config.h even earlier to avoid any possibility that such renaming lands too late; config.h should at this point precede any declarations.

* odbc_sql2tds: Treat SYB5INT8 like other numeric types. * tds_generic_get: Handle non-blob types with four-byte sizes (e.g., SYBLONG{BINARY,CHAR}). * tds_generic_put_info: Send size 255 (0xff) for NULL short character types in output columns. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

* tds.h (is_blob_col): Consider column_type in addition to column_varint_size. * Retire 5 as a nominal column_varint_size value in favor of unified logic. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

- Harmonize variable, parameter, etc. data types more fully. - Adjust system API usage accordingly: -- For some string constants, use sizeof() - 1 in lieu of strlen, taking care to ensure they come from array variables. -- Keep format specifiers for printf et al. in sync with argument types. -- dbprrow: Account for the fact that printf always uses int for dynamic widths by substituting memchr + fwrite. -- odbc/prepare_query.c (prepared_rpc): Substitute atoi for strtol (already used with no endptr) when populating TDSINT4. - Cast away some remaining discrepancies where safe, in some cases by taking advantage of TDS_PUT_* macros. - ctlib.h: Explicitly define _CS_CURS_TYPE_* in terms of TDS_CURSOR_STATE_* rather than duplicating their values. - odbc_util.c: #include <stddef.h> for ptrdiff_t (now used in odbc_set_string_flag). - tds_parse_conf_section: To legitimize a cast, tighten debug_flags' range check on systems where int is narrower than long. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

All could at least potentially receive updates, but they were never consulted or even logged. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

freddy77 reviewed Jun 2, 2024

View reviewed changes

freddy77 reviewed Jan 30, 2025

View reviewed changes

src/tds/ncbi_strerror.c Outdated Show resolved Hide resolved

freddy77 reviewed Jan 30, 2025

View reviewed changes

src/tds/sspi.c Outdated Show resolved Hide resolved

freddy77 reviewed Jan 30, 2025

View reviewed changes

src/tds/util.c Outdated Show resolved Hide resolved

ucko added 5 commits February 13, 2025 11:42

Predefine _FREETDS_LIBRARY_SOURCE for all library code.

6adbeea

This distinction will initially be useful only for ctlib and dblib, but applying it across the board may avoid possible confusion if it turns out to be useful elsewhere too. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

Improve TDS 5 (Sybase) LONGBINARY and LONGCHAR handling.

bfbeac7

* tds.h (is_blob_col): Consider column_type in addition to column_varint_size. * Retire 5 as a nominal column_varint_size value in favor of unified logic. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

_ct_handle_client_message: Report system errors' descriptions.

dff654c

Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

ucko force-pushed the ncbi-2024-05-misc-improvements branch from 58c9ae5 to cc03c6d Compare February 14, 2025 20:35

ucko added 2 commits February 14, 2025 16:29

Retire unread variables.

c86e6ee

All could at least potentially receive updates, but they were never consulted or even logged. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

ucko force-pushed the ncbi-2024-05-misc-improvements branch from cc03c6d to c86e6ee Compare February 14, 2025 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assorted improvements from NCBI as of May 2024 #559

Assorted improvements from NCBI as of May 2024 #559

ucko commented May 30, 2024

freddy77 Jun 2, 2024

ucko Jun 3, 2024

freddy77 Jun 3, 2024

freddy77 Jan 30, 2025

ucko Feb 14, 2025

freddy77 Feb 15, 2025

ucko Feb 18, 2025

freddy77 Jan 30, 2025

ucko Feb 14, 2025

freddy77 Jan 30, 2025

ucko Feb 14, 2025

freddy77 Jan 30, 2025

ucko Feb 14, 2025

Assorted improvements from NCBI as of May 2024 #559

Are you sure you want to change the base?

Assorted improvements from NCBI as of May 2024 #559

Conversation

ucko commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment