[Libdbi-drivers-devel] freetds driver, single instance?

2008-08-05 Thread Balazs Scheidler
Hi,

As it seems we're a bit unlucky at these times. Now I've ran into a
problem in the FreeTDS driver, we're getting heap corruptions inside
freetds, and as it seems the root cause is the DBD freetds driver.

More exactly, dbd_freetds seems to have a single state instance, instead
of allocating a new state for each connection:

typedef struct freedts_type {
CS_CONTEXT *ctx;
CS_CONNECTION *conn;
CS_COMMAND *cmd;
} FREETDSCON;
static FREETDSCON freetds;
^

int dbd_connect(dbi_conn_t * conn)
{
FREETDSCON *tdscon = freetds;


This means that if an application uses two DBI contexts at the same
time, those will clash and cause a segfault.

Does anyone know why this limitation was added to the freetds driver?

I'm experimenting with the following patch, but still testing it. I'd
appreciate if you could review it for mistakes I possibly made.

BTW: what is the policy about commented out code in
libdbi/libdbi-drivers source? I personally feel that commented out code
is not good practice, especially if the code in question is commented
out without a note stating the exact reason why it was done. For me it
makes the code harder to read.

-- 
Bazsi
commit 6ec70cf9b3d780a5cceb584ad28ded7a6ec31196
Author: Balazs Scheidler [EMAIL PROTECTED]
Date:   Tue Aug 5 10:59:19 2008 +0200

allow the use of multiple parallel freetds connections

diff --git a/drivers/freetds/dbd_freetds.c b/drivers/freetds/dbd_freetds.c
index 449c313..b869e8b 100644
--- a/drivers/freetds/dbd_freetds.c
+++ b/drivers/freetds/dbd_freetds.c
@@ -56,7 +56,6 @@ typedef struct freedts_type {
 CS_CONNECTION *conn;
 CS_COMMAND *cmd;
 } FREETDSCON;
-static FREETDSCON freetds;
 
 static const dbi_info_t driver_info = {
 freetds,
@@ -150,12 +149,13 @@ int dbd_initialize(dbi_driver_t * driver)
 
 int dbd_connect(dbi_conn_t * conn)
 {
-
-FREETDSCON *tdscon = freetds;
+FREETDSCON *tdscon;
 CS_RETCODE ret;
 
 char *str;
 unsigned int num;
+
+tdscon = malloc(sizeof(FREETDSCON));
 /*
  * Allocate memory for structs
  */
@@ -174,7 +174,7 @@ int dbd_connect(dbi_conn_t * conn)
/* Deallocate ctx struct */
cs_ctx_drop(tdscon-ctx);
 }
-
+free(tdscon);
 return -1;
   success_allocate:
 conn-connection = tdscon;
@@ -273,7 +273,7 @@ int dbd_disconnect(dbi_conn_t * conn)
 ct_con_drop(tdscon-conn);
 ct_exit(tdscon-ctx, CS_UNUSED);
 cs_ctx_drop(tdscon-ctx);
-
+free(tdscon);
 return 0;
 }
 
-
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK  win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100url=/___
Libdbi-drivers-devel mailing list
Libdbi-drivers-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libdbi-drivers-devel


Re: [Libdbi-drivers-devel] freetds driver, single instance?

2008-08-05 Thread Markus Hoenicka
Quoting Balazs Scheidler [EMAIL PROTECTED]:

 Hi,

 As it seems we're a bit unlucky at these times. Now I've ran into a
 problem in the FreeTDS driver, we're getting heap corruptions inside
 freetds, and as it seems the root cause is the DBD freetds driver.

 More exactly, dbd_freetds seems to have a single state instance, instead
 of allocating a new state for each connection:

 typedef struct freedts_type {
 CS_CONTEXT *ctx;
 CS_CONNECTION *conn;
 CS_COMMAND *cmd;
 } FREETDSCON;
 static FREETDSCON freetds;
 ^


This is certainly against the idea of being able to create as many  
connections as you need. Afaict no other driver suffers from such a  
limitation.


 Does anyone know why this limitation was added to the freetds driver?


I'm afraid no. Vadym (who developed the driver) is no longer active as  
he does not seem to work with MS SQL Server anymore, but I'll forward  
your mail to him and see whether he can explain.

 I'm experimenting with the following patch, but still testing it. I'd
 appreciate if you could review it for mistakes I possibly made.


Your patch looks good to me although I can't test-drive it.

 BTW: what is the policy about commented out code in
 libdbi/libdbi-drivers source? I personally feel that commented out code
 is not good practice, especially if the code in question is commented
 out without a note stating the exact reason why it was done. For me it
 makes the code harder to read.


There is no such thing as a policy right now. If anyone feels like we  
should have a policy, I'll be happy to help finding or writing one.  
There are quite a few things that we could improve, from aesthetic (C  
vs. C++ style comments) to good practice (variable names, comments  
describing the usage and parameter list of functions). I use  
commented-out code mainly for printf() debugging which I personally  
feel is reasonable. I agree that blocks of commented-out code should  
be avoided or at least explained and signed.

regards,
Markus

-- 
Markus Hoenicka
[EMAIL PROTECTED]
(Spam-protected email: replace the quadrupeds with mhoenicka)
http://www.mhoenicka.de


-
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK  win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100url=/
___
Libdbi-drivers-devel mailing list
Libdbi-drivers-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libdbi-drivers-devel


Re: [Libdbi-drivers-devel] freetds driver, single instance?

2008-08-05 Thread Balazs Scheidler
On Tue, 2008-08-05 at 12:35 +0200, Markus Hoenicka wrote:
 Quoting Balazs Scheidler [EMAIL PROTECTED]:
 
  Hi,
 
  As it seems we're a bit unlucky at these times. Now I've ran into a
  problem in the FreeTDS driver, we're getting heap corruptions inside
  freetds, and as it seems the root cause is the DBD freetds driver.
 
  More exactly, dbd_freetds seems to have a single state instance, instead
  of allocating a new state for each connection:
 
  typedef struct freedts_type {
  CS_CONTEXT *ctx;
  CS_CONNECTION *conn;
  CS_COMMAND *cmd;
  } FREETDSCON;
  static FREETDSCON freetds;
  ^
 
 
 This is certainly against the idea of being able to create as many  
 connections as you need. Afaict no other driver suffers from such a  
 limitation.
 
 
  Does anyone know why this limitation was added to the freetds driver?
 
 
 I'm afraid no. Vadym (who developed the driver) is no longer active as  
 he does not seem to work with MS SQL Server anymore, but I'll forward  
 your mail to him and see whether he can explain.
 
  I'm experimenting with the following patch, but still testing it. I'd
  appreciate if you could review it for mistakes I possibly made.
 
 
 Your patch looks good to me although I can't test-drive it.

It works with light testing, e.g. now it does not crash where it used
to. I did not check for leaks or similar stuff, however I think I only
needed to free the allocated memory in dbd_disconnect() and in case of
an error. I've added both of these spots, so it should be ok in this
regard. We are installing the patch in production, but now I think it is
safe to commit it.

 
  BTW: what is the policy about commented out code in
  libdbi/libdbi-drivers source? I personally feel that commented out code
  is not good practice, especially if the code in question is commented
  out without a note stating the exact reason why it was done. For me it
  makes the code harder to read.
 
 
 There is no such thing as a policy right now. If anyone feels like we  
 should have a policy, I'll be happy to help finding or writing one.  
 There are quite a few things that we could improve, from aesthetic (C  
 vs. C++ style comments) to good practice (variable names, comments  
 describing the usage and parameter list of functions). I use  
 commented-out code mainly for printf() debugging which I personally  
 feel is reasonable. I agree that blocks of commented-out code should  
 be avoided or at least explained and signed.

Just let me know if you accept patches that remove these kind of code
blocks. (for instance the oracle compilation problem we fixed last
contained some dead code that I didn't remove, as I didn't want to be
too intrusive. if it was my code, I would have removed those as well).

Another completely unrelated topic: have you thought of using git
instead of CVS? It would be so much simpler to contribute :)

-- 
Bazsi



-
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK  win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100url=/
___
Libdbi-drivers-devel mailing list
Libdbi-drivers-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libdbi-drivers-devel