FreeTDS Development Group

[Frediano Ziglio <freddy77 AT gmail.com>]

Even gmail seems to dislike text mail...

See https://gitorious.org/freetds/mars-freetds/commits/config_error

I think it would be great to integrate the tdserror stuff so patch
would be more verbose.

Frediano

2013/10/22 Marc Abramowitz <msabramo AT gmail.com>:
> That looks fairly similar but better than mine as you put the invalid_flag
> in the tds_login struct instead of a global.
>
> If you email me a file with a patch that's based off master, I can try it
> out if you'd like. I had trouble copying and pasting the patch because of
> line breaks (from Gmail?) and even after fixing those, the patch didn't
> seem to apply cleanly. I'd be glad to try it though.
>
>
> On Tue, Oct 22, 2013 at 1:13 PM, Frediano Ziglio <freddy77 AT gmail.com> wrote:
>
>> I wrote something like this
>>
>> Date: Tue, 22 Oct 2013 21:12:13 +0100
>> Subject: [PATCH] Simple and probably incomplete patch to check
>> configuration
>>  values.
>>
>> Signed-off-by: Frediano Ziglio <freddy77 AT gmail.com>
>> ---
>>  include/freetds/tds.h    |    3 ++-
>>  src/odbc/connectparams.c |    8 ++++----
>>  src/tds/config.c         |   22 +++++++++++++---------
>>  src/tds/login.c          |    3 +++
>>  4 files changed, 22 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/freetds/tds.h b/include/freetds/tds.h
>> index b27fcf6..f4a2621 100644
>> --- a/include/freetds/tds.h
>> +++ b/include/freetds/tds.h
>> @@ -578,6 +578,7 @@ typedef struct tds_login
>>      unsigned int use_ntlmv2:1;
>>      unsigned int use_lanman:1;
>>      unsigned int mars:1;
>> +    unsigned int invalid_configuration:1;
>>  } TDSLOGIN;
>>
>>  typedef struct tds_locale
>> @@ -1187,7 +1188,7 @@ const char *tds_addrinfo2str(struct tds_addrinfo
>> *addr, char *name, int namemax)
>>
>>  TDSRET tds_set_interfaces_file_loc(const char *interfloc);
>>  extern const char STD_DATETIME_FMT[];
>> -int tds_config_boolean(const char *value);
>> +int tds_config_boolean(const char *value, TDSLOGIN * login);
>>
>>  TDSLOCALE *tds_get_locale(void);
>>  TDSRET tds_alloc_row(TDSRESULTINFO * res_info);
>> diff --git a/src/odbc/connectparams.c b/src/odbc/connectparams.c
>> index b287311..0a910d3 100644
>> --- a/src/odbc/connectparams.c
>> +++ b/src/odbc/connectparams.c
>> @@ -226,12 +226,12 @@ odbc_get_dsn_info(TDS_ERRS *errs, const char
>> *DSN, TDSLOGIN * login)
>>      if (myGetPrivateProfileString(DSN, odbc_param_ServerSPN, tmp) > 0)
>>          tds_parse_conf_section(TDS_STR_SPN, tmp, login);
>>
>> -    if (myGetPrivateProfileString(DSN, odbc_param_Trusted_Connection,
>> tmp) > 0 && tds_config_boolean(tmp)) {
>> +    if (myGetPrivateProfileString(DSN, odbc_param_Trusted_Connection,
>> tmp) > 0 && tds_config_boolean(tmp, login)) {
>>          tds_dstr_copy(&login->user_name, "");
>>          tds_dstr_copy(&login->password, "");
>>      }
>>
>> -    if (myGetPrivateProfileString(DSN, odbc_param_MARS_Connection,
>> tmp) > 0 && tds_config_boolean(tmp)) {
>> +    if (myGetPrivateProfileString(DSN, odbc_param_MARS_Connection,
>> tmp) > 0 && tds_config_boolean(tmp, login)) {
>>          login->mars = 1;
>>      }
>>
>> @@ -389,12 +389,12 @@ odbc_parse_connect_string(TDS_ERRS *errs, const
>> char *connect_string, const char
>>          } else if (CHK_PARAM(ServerSPN)) {
>>              tds_parse_conf_section(TDS_STR_SPN, tds_dstr_cstr(&value),
>> login);
>>          } else if (CHK_PARAM(Trusted_Connection)) {
>> -            trusted = tds_config_boolean(tds_dstr_cstr(&value));
>> +            trusted = tds_config_boolean(tds_dstr_cstr(&value), login);
>>              tdsdump_log(TDS_DBG_INFO1, "trusted %s -> %d\n",
>> tds_dstr_cstr(&value), trusted);
>>              num_param = -1;
>>              /* TODO odbc_param_Address field */
>>          } else if (CHK_PARAM(MARS_Connection)) {
>> -            if (tds_config_boolean(tds_dstr_cstr(&value)))
>> +            if (tds_config_boolean(tds_dstr_cstr(&value), login))
>>                  login->mars = 1;
>>          }
>>
>> diff --git a/src/tds/config.c b/src/tds/config.c
>> index c4d38e7..41386e5 100644
>> --- a/src/tds/config.c
>> +++ b/src/tds/config.c
>> @@ -399,6 +399,8 @@ tds_read_conf_sections(FILE * in, const char
>> *server, TDSLOGIN * login)
>>      default_port = login->port;
>>
>>      found = tds_read_conf_section(in, server, tds_parse_conf_section,
>> login);
>> +    if (login->invalid_configuration)
>> +        return 0;
>>
>>      /*
>>       * If both instance and port are specified and neither one came
>> from the default, it's an error
>> @@ -428,7 +430,7 @@ static const struct {
>>  };
>>
>>  int
>> -tds_config_boolean(const char *value)
>> +tds_config_boolean(const char *value, TDSLOGIN *login)
>>  {
>>      int p;
>>
>> @@ -436,7 +438,7 @@ tds_config_boolean(const char *value)
>>          if (!strcasecmp(value, boolean_values[p].value))
>>              return boolean_values[p].to_return;
>>      }
>> -    tdsdump_log(TDS_DBG_INFO1, "UNRECOGNIZED boolean value: '%s'.
>> Treating as 'no'.\n", value);
>> +    login->invalid_configuration = 1;
>>      return 0;
>>  }
>>
>> @@ -451,8 +453,10 @@ tds_config_encryption(const char * value, TDSLOGIN *
>> login)
>>          lvl = TDS_ENCRYPTION_REQUEST;
>>      else if (!strcasecmp(value, TDS_STR_ENCRYPTION_REQUIRE))
>>          lvl = TDS_ENCRYPTION_REQUIRE;
>> -    else
>> +    else {
>>          tdsdump_log(TDS_DBG_INFO1, "UNRECOGNIZED option value
>> '%s'...ignoring.\n", value);
>> +        login->invalid_configuration = 1;
>> +    }
>>
>>      login->encryption_level = lvl;
>>  }
>> @@ -568,10 +572,10 @@ tds_parse_conf_section(const char *option, const
>> char *value, void *param)
>>          if (val >= 512 && val < 65536)
>>              login->block_size = val;
>>      } else if (!strcmp(option, TDS_STR_SWAPDT)) {
>> -        login->broken_dates = tds_config_boolean(value);
>> +        login->broken_dates = tds_config_boolean(value, login);
>>      } else if (!strcmp(option, TDS_GSSAPI_DELEGATION)) {
>>          /* gssapi flag addition */
>> -        login->gssapi_use_delegation = tds_config_boolean(value);
>> +        login->gssapi_use_delegation = tds_config_boolean(value, login);
>>      } else if (!strcmp(option, TDS_STR_DUMPFILE)) {
>>          tds_dstr_copy(&login->dump_file, value);
>>      } else if (!strcmp(option, TDS_STR_DEBUGFLAGS)) {
>> @@ -604,7 +608,7 @@ tds_parse_conf_section(const char *option, const
>> char *value, void *param)
>>          if (atoi(value))
>>              login->port = atoi(value);
>>      } else if (!strcmp(option, TDS_STR_EMUL_LE)) {
>> -        login->emul_little_endian = tds_config_boolean(value);
>> +        login->emul_little_endian = tds_config_boolean(value, login);
>>      } else if (!strcmp(option, TDS_STR_TEXTSZ)) {
>>          if (atoi(value))
>>              login->text_size = atoi(value);
>> @@ -617,7 +621,7 @@ tds_parse_conf_section(const char *option, const
>> char *value, void *param)
>>      } else if (!strcmp(option, TDS_STR_LANGUAGE)) {
>>          tds_dstr_copy(&login->language, value);
>>      } else if (!strcmp(option, TDS_STR_APPENDMODE)) {
>> -        tds_g_append_mode = tds_config_boolean(value);
>> +        tds_g_append_mode = tds_config_boolean(value, login);
>>      } else if (!strcmp(option, TDS_STR_INSTANCE)) {
>>          tds_dstr_copy(&login->instance_name, value);
>>      } else if (!strcmp(option, TDS_STR_ENCRYPTION)) {
>> @@ -625,9 +629,9 @@ tds_parse_conf_section(const char *option, const
>> char *value, void *param)
>>      } else if (!strcmp(option, TDS_STR_ASA_DATABASE)) {
>>          tds_dstr_copy(&login->server_name, value);
>>      } else if (!strcmp(option, TDS_STR_USENTLMV2)) {
>> -        login->use_ntlmv2 = tds_config_boolean(value);
>> +        login->use_ntlmv2 = tds_config_boolean(value, login);
>>      } else if (!strcmp(option, TDS_STR_USELANMAN)) {
>> -        login->use_lanman = tds_config_boolean(value);
>> +        login->use_lanman = tds_config_boolean(value, login);
>>      } else if (!strcmp(option, TDS_STR_REALM)) {
>>          tds_dstr_copy(&login->server_realm_name, value);
>>      } else if (!strcmp(option, TDS_STR_SPN)) {
>> diff --git a/src/tds/login.c b/src/tds/login.c
>> index ec8eb5a..813290b 100644
>> --- a/src/tds/login.c
>> +++ b/src/tds/login.c
>> @@ -333,6 +333,9 @@ tds_connect(TDSSOCKET * tds, TDSLOGIN * login, int
>> *p_oserr)
>>          , 0x402
>>          };
>>
>> +    if (login->invalid_configuration)
>> +        return TDS_FAIL;
>> +
>>      if (TDS_MAJOR(login) == 0) {
>>          unsigned int i;
>>          TDSSAVECONTEXT save_ctx;
>> --
>> 1.7.9.5
>>
>> 2013/10/22 Marc Abramowitz <msabramo AT gmail.com>:
>> >> I like tdserror. I don't like printf or an additional option. Printf
>> > cause it
>> >> can go nowhere or in dangerous places, an additional option cause it
>> force
>> >> different users to recompile the same library with different options
>> > based on
>> >> the usage. Using options to deal with debugging or different system
>> >> configuration is fine but this option would be ok for an application but
>> > not
>> >> for a library used in different places.
>> >
>> >> What I would do, also suggested by James is something strong. If you
>> pass
>> > a
>> >> wrong option just try to notify upper layer (as tdserror does) and do
>> not
>> >> connect! Surely user will fix the configuration! Of course should be
>> >> documented in the version changelog and UG. I'll try to write a partial,
>> > not
>> >> that tested patch. Does it sound reasonable?
>> >
>> > I had another branch where I did something like that actually. I just
>> > cleaned it up and made another merge request.
>> >
>> > https://gitorious.org/freetds/freetds/merge_requests/18
>> >
>> > If that resembles or is useful towards implementing what you suggested,
>> > feel free to use it, or if it's different, you can go ahead and do your
>> own
>> > thing. The idea certainly sounds reasonable.
>> >
>> >
>> > On Mon, Oct 21, 2013 at 4:51 PM, Frediano Ziglio <freddy77 AT gmail.com>
>> wrote:
>> >
>> >> I like tdserror. I don't like printf or an additional option. Printf
>> cause
>> >> it can go nowhere or in dangerous places, an additional option cause it
>> >> force different users to recompile the same library with different
>> options
>> >> based on the usage. Using options to deal with debugging or different
>> >> system configuration is fine but this option would be ok for an
>> application
>> >> but not for a library used in different places.
>> >>
>> >> What I would do, also suggested by James is something strong. If you
>> pass
>> >> a wrong option just try to notify upper layer (as tdserror does) and do
>> not
>> >> connect! Surely user will fix the configuration! Of course should be
>> >> documented in the version changelog and UG. I'll try to write a partial,
>> >> not that tested patch. Does it sound reasonable?
>> >>
>> >> Ps: we have still to sort out the Travis patch. Does the configuration
>> >> file need to be in the main directory? Cannot you change the name? What
>> >> happen if another developers want to use Travis too?
>> >>
>> >> Frediano Ziglio
>> >>
>> >> Il giorno 21/ott/2013, alle ore 21:48, Marc Abramowitz <
>> msabramo AT gmail.com>
>> >> ha scritto:
>> >>
>> >> > That makes sense.
>> >> >
>> >> > OK, I took another stab at creating a louder notification when the
>> >> > encryption setting is incorrect:
>> >> >
>> >> > *https://gitorious.org/freetds/freetds/merge_requests/16* ("Improve
>> >> > handling of invalid encryption setting")
>> >> >
>> >> > The goal here is to make it much more noticeable when there is an
>> invalid
>> >> > encryption setting. I do this by:
>> >> >
>> >> >   - calling tdserror with a new error code (TDSECONF = 20214 = "Local
>> >> >   configuration error")
>> >> >   - print a message to stderr in case in case someone is looking there
>> >> >   - exiting the process if the ENABLE_EXIT_ON_INVALID_CONFIG is
>> defined
>> >> >   (defined by passing --enable-exit-on-invalid-config to./configure).
>> >> >
>> >> >
>> >> >
>> >> > On Sat, Oct 19, 2013 at 11:49 AM, James K. Lowden <
>> jklowden AT freetds.org
>> >> >wrote:
>> >> >
>> >> >> On Sat, 19 Oct 2013 09:48:12 -0700
>> >> >> Marc Abramowitz <msabramo AT gmail.com> wrote:
>> >> >>
>> >> >>>>> Merge request to make an invalid encryption setting a fatal error:
>> >> >>>>> https://gitorious.org/freetds/freetds/merge_requests/10
>> >> >>>>
>> >> >>>> Ouch indeed.  I think the next release of FreeTDS should probably
>> >> >>>> reject all incorrect configurations, both invalid keywords and
>> >> >>>> out-of-range values.  Diagnosing configuration errors is easier if
>> >> >>>> the error is obvious, and the remedy is simple.
>> >> >>>
>> >> >>> Freddy pointed out that my merge request wasn't acceptable because
>> >> >>> printing to stderr and exit aren't appropriate for a library.
>> >> >> ...
>> >> >>> Is there some middle ground - some way of indicating a config error
>> >> >>> (esp. a serious config error like an invalid encryption setting)
>> that
>> >> >>> is very noticeable but doesn't overstep what a library should do?
>> >> >>
>> >> >> The Right Way is to hook into the error handler.  Add something like
>> >> >> this to the table in sybdb.h:
>> >> >>
>> >> >> #define SYBECONF         2501   /* local configuration error */
>> >> >>
>> >> >> and to the dblib.c::dblib_error_messages table
>> >> >>
>> >> >> and invoke that error with tdserror or dbperror.
>> >> >>
>> >> >> It's not obvious to me that printing to standard error and exiting is
>> >> >> all that bad, though.
>> >> >>
>> >> >> In general, there are two kinds of errors:
>> >> >>
>> >> >> 1.  logic errors, caused by the programmer getting something wrong.
>> >> >> Examples include off-by-one errors and failure to check for a NULL
>> >> >> pointer.  These kinds of problems can and do cause the library to
>> >> >> exit via  e.g. assert(3).  There is no point in continuing, because
>> the
>> >> >> program has reached an "impossible" state that the program did not
>> >> >> anticipate and thus cannot handle.
>> >> >>
>> >> >> 2.  runtime errors, caused by invalid user input or dynamic failures
>> >> >> such as network disconnection or the server not responding.  These
>> >> >> should always return an error to the user, who is the only person in
>> a
>> >> >> position to deal with it.
>> >> >>
>> >> >> Is an invalid configuration a logic or runtime error?  is the
>> >> >> administrator a programmer or a user?
>> >> >>
>> >> >> I would say the configuration file is more like program text.  It is
>> >> >> static data isolated from the program in such a way that it can be
>> >> >> modified without requiring recompilation.  The very first time it is
>> >> >> read -- compiled, if you will -- it will be validated.  If it is
>> >> >> invalid, the library exits.  Once the configuration is valid, no
>> amount
>> >> >> of other invalid input will cause it to become invalid.
>> >> >>
>> >> >> Printing to stderr is unfortunately not robust, because some
>> >> >> applications will not have that descriptor open.  Invoking the error
>> >> >> handler is is more general, but will go unseen if the application
>> >> >> doesn't install and error handler. On the other hand, the problem
>> can be
>> >> >> instantantly diagnosed by using any command-line tool such as tsql.
>> >> >>
>> >> >> So, IMO printing to stderr and exiting is fine, and invoking the
>> error
>> >> >> handler is in some ways friendlier (perhaps in addition to writing to
>> >> >> stderr).  Exiting is OK, too, even if it does force the admin to
>> clean
>> >> >> up errors that might otherwise be innocuous.
>> >> >>
>> >> >> HTH.
>> >> >>
>> >> >> --jkl
>> >> >> _______________________________________________
>> >> >> FreeTDS mailing list
>> >> >> FreeTDS AT lists.ibiblio.org
>> >> >> http://lists.ibiblio.org/mailman/listinfo/freetds
>> >> >>
>> >> > _______________________________________________
>> >> > FreeTDS mailing list
>> >> > FreeTDS AT lists.ibiblio.org
>> >> > http://lists.ibiblio.org/mailman/listinfo/freetds
>> >> _______________________________________________
>> >> FreeTDS mailing list
>> >> FreeTDS AT lists.ibiblio.org
>> >> http://lists.ibiblio.org/mailman/listinfo/freetds
>> >>
>> > _______________________________________________
>> > FreeTDS mailing list
>> > FreeTDS AT lists.ibiblio.org
>> > http://lists.ibiblio.org/mailman/listinfo/freetds
>> _______________________________________________
>> FreeTDS mailing list
>> FreeTDS AT lists.ibiblio.org
>> http://lists.ibiblio.org/mailman/listinfo/freetds
>>
> _______________________________________________
> FreeTDS mailing list
> FreeTDS AT lists.ibiblio.org
> http://lists.ibiblio.org/mailman/listinfo/freetds