Add processing of connection strings and DSNs management #1
Conversation
driver/connect.c
Outdated
| #define CONNSTR_KW_MAX_FETCH_SIZE "MaxFetchSize" | ||
| #define CONNSTR_KW_MAX_BODY_SIZE "MaxBodySize" | ||
| #define CONNSTR_KW_TRACE_FILE "TraceFile" | ||
| #define CONNSTR_KW_TRACE_LEVEL "TraceLevel" |
There was a problem hiding this comment.
I haven't had time for a proper review yet, but one thing that immediately stands out is that you're using tabs for indenting, which (as the way the GitHub diff tool has chosen to lay out the code above highlights) looks different depending on the viewer's tab settings.
Is there a good reason not to switch to spaces? Both the ML C++ code and the Elasticsearch Java code use 4 spaces per indent.
If you think it's a good idea then /usr/bin/expand could make the changes.
There was a problem hiding this comment.
Yes, that's a grassroots issue . :-)
I guess this is generally a matter of preference, if consistency is kept.
Myself, I prefer tabs for the ("classical") reasons of:
- avoiding the debate if tabs should be 2 (ex. libcurl), 4 or even 8 (part of ODBC API, most being 4 though; linux kernel) spaces wide, and/or how that plays with a line width of 80 characters, which I've also been keeping (vs 140, the limit in Elasticsearch, though interestingly, the proprietary license is also 80 chars wide, but like most licenses otherwise; but arguably, OO languages tend to "generate" longer lines).
- most editors being able to expand the tabs to a preferred size (unlike the reverse transformation).
- tabs being meant for indentation and tabulating.
Complementary to that, I document the editor settings at the bottom of every source code file.
Now that given, I'm not that fussy about it; my preference is with the standard I currently use, but I understand there are reasons for spaces instead of tabs (diffs can be less easy to read, especially in a browser, code is sometimes viewed with simple shells editors etc.) and I'd be fine with a switch-over should we decide that's worth it.
There was a problem hiding this comment.
OK fair enough. It's your repo so you get to decide the style. I just wanted to make sure you were aware that all the major elastic repos are using spaces.
SQLTCHAR, belonging to ODBC API, expands to SQLWCHAR when UNICODE compiling, SQLCHAR otherwise. This was used in the hope for an easy concurrent development of a Unicode and ANSI driver, or at least facilitate later the ANSI version development. However, this was a naive assumption, has been used inconsistently and would have generated wrong code in some places, with no-Unicode compilation. The commit touches much code, but is just simple textual substitution: - s/SQLTCHAR/SQLWCHAR/ - s/\<tstr\>/wptr/ - s/MK_TSTR/MK_WSTR/ - S/MK_WSTR/MK_WPTR/ (This will also facilitate adding next a much needed wstr struct for defining non-zero ended wchar_t strings with lenght.)
added new wstr_st struct for SQLWCHAR non 0-terminated strings
add util.c/.h module
Rework and progress of the function to
- consider the values of "DriverCompletion" parameter:
- promt the user for more info (potentially repetitively until
connection is established);
- conider the system registry;
- discriminate between DRIVER and DSN configuration;
- correctly assembly the connection string that would eventually be
writen into a file DSN.
Missing still:
- reading the system registry;
- the graphical part / user prompting.
Read registry information to fetch driver configuration attributes, in case the connection string contains the DSN attribute with a valid value.
driver/connect.c
Outdated
| return TRUE; | ||
| } | ||
|
|
||
| static BOOL as_long(size_t cnt, SQLWCHAR *val, long *out) | ||
| static BOOL wstr2long(wstr_st *val, long *out) |
There was a problem hiding this comment.
I'm curious why you've re-implemented this function yourself. Was it for performance reasons?
There was a problem hiding this comment.
A rather minor concern was performance as well; currently the function is used for reading the config only, so performance isn't an issue, but I hope to reuse it later for data conversions (i.e. data received as string from ES, but consuming app wants a long/int), which the driver is required to do.
The main reason though is wcstol()'s need for a null terminated string, which I could only provide either by modifying the original string (zero it - convert - restore it) or duplicating it (or copying it in a local buffer to avoid allocation).
I have - arguably - considered a "quick" implementation as cleaner.
There was a problem hiding this comment.
This looks fine to me Bogdan.
As a matter of style I would have preferred that the body of single line if/else statements were always enclosed in braces. I see that it has been done in a few places but not consistently. I'm not overly concerned about the matter though..
| cleanup_curl(dbc); | ||
| if (abuff) /* if buffer had been set, the error occured in _perform() */ | ||
| if (abuff) { | ||
| free(abuff); |
There was a problem hiding this comment.
Assign buff to NULL here? After freeing
There was a problem hiding this comment.
Sure, committed the change in f0371e0.
It's a local variable freed in the error case at the end of the function, so it shouldn't be reused, but NULL'ing it is good practice (in case the code block would be reused) and the compiler can easily optimize the statement out when needed.
keep the check against null safe, in case the code block gets shuffled
SQLTCHAR, belonging to ODBC API, expands to SQLWCHAR when UNICODE compiling, SQLCHAR otherwise. This was used in the hope for an easy concurrent development of a Unicode and ANSI driver, or at least facilitate later the ANSI version development. However, this was a naive assumption, has been used inconsistently and would have generated wrong code in some places, with no-Unicode compilation. The commit touches much code, but is just simple textual substitution: - s/SQLTCHAR/SQLWCHAR/ - s/\<tstr\>/wptr/ - s/MK_TSTR/MK_WSTR/ - S/MK_WSTR/MK_WPTR/ (This will also facilitate adding next a much needed wstr struct for defining non-zero ended wchar_t strings with lenght.)
added new wstr_st struct for SQLWCHAR non 0-terminated strings
Add processing of connection strings and DSNs management
The driver receives it's configuration from the Driver Manager (DM) through connection strings.
These connection strings can be exhaustive, providing full information on the server to connect to, how to interact with it, how to treat the data etc. or partial, requiring the driver to retrieve information about the data source from various possible sources (system information, files, user prompts).
Supporting DSNs means: