You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by bo...@apache.org on 2006/06/07 04:04:44 UTC
svn commit: r412252 - /apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Author: bojan
Date: Tue Jun 6 19:04:43 2006
New Revision: 412252
URL: http://svn.apache.org/viewvc?rev=412252&view=rev
Log:
Properly free results table.
Modified:
apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Modified: apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
URL: http://svn.apache.org/viewvc/apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c?rev=412252&r1=412251&r2=412252&view=diff
==============================================================================
--- apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c (original)
+++ apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c Tue Jun 6 19:04:43 2006
@@ -97,7 +97,7 @@
(*results)->random = seek;
if (tuples > 0)
- apr_pool_cleanup_register(pool, *result, (void *) free,
+ apr_pool_cleanup_register(pool, result, (void *) sqlite_free_table,
apr_pool_cleanup_null);
ret = 0;
@@ -307,7 +307,7 @@
{
apr_dbd_t *sql;
sqlite *conn = NULL;
- char *filename, *perm;
+ char *perm;
int iperms = 600;
char* params = apr_pstrdup(pool, params_);
/* params = "[filename]:[permissions]"
Re: svn commit: r412252 -
/apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Bojan Smojver <bo...@rexursive.com>:
> The following code, from apr_dbd.c, probably won't have an easy fix
> like the drivers did:
The only thing that springs to mind is to change apr_dbd_internal.h to read:
apr_status_t (*end_transaction)(void *trans);
And then patch individual drivers. However, return status from private
end_transaction functions is not (at present) an APR status code, so
we would still be cheating... A bit ;-)
--
Bojan
Re: svn commit: r412252 -
/apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Joe Orton <jo...@redhat.com>:
> This is really still a long way from "properly" - both the old and new
> code has undefined behaviour because of the function cast.
The following code, from apr_dbd.c, probably won't have an easy fix
like the drivers did:
--------------------------------------------------
#define CLEANUP_CAST (apr_status_t (*)(void*))
[..snip..]
APU_DECLARE(int) apr_dbd_transaction_start(const apr_dbd_driver_t *driver,
apr_pool_t *pool,
apr_dbd_t *handle,
apr_dbd_transaction_t **trans)
{
int ret = driver->start_transaction(pool, handle, trans);
if (*trans) {
apr_pool_cleanup_register(pool, *trans,
CLEANUP_CAST driver->end_transaction,
apr_pool_cleanup_null);
}
return ret;
}
APU_DECLARE(int) apr_dbd_transaction_end(const apr_dbd_driver_t *driver,
apr_pool_t *pool,
apr_dbd_transaction_t *trans)
{
apr_pool_cleanup_kill(pool, trans, CLEANUP_CAST driver->end_transaction);
return driver->end_transaction(trans);
}
--------------------------------------------------
We are just lucky here that driver->end_transaction() takes a txn
pointer and returns an int, which is equivalent for most intents and
puposes to (apr_status_t (*)(void*)).
--
Bojan
Re: svn commit: r412252 - /apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2006-06-07 at 12:58 +0100, Joe Orton wrote:
> This is not really a concern with use of sqlite2, but I think it's a
> good idea in general ;)
Yep, seems clever to me.
--
Bojan
Re: svn commit: r412252 - /apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jun 07, 2006 at 09:50:06PM +1000, Bojan Smojver wrote:
> On Wed, 2006-06-07 at 12:44 +0100, Joe Orton wrote:
>
> > Another useful thing that can be done in general is to do e.g.
> >
> > static apr_status_t blah(void *data)
> > {
> > char *foo = data;
> > the_function(foo)
> >
> > since passing a variable of the right type to the function rather than
> > just void * gives better type-safety.
>
> Is this something that inherent in C spec, or is it something that some
> compilers tend to screw up?
Oh, it's just that you get type-checking against the argument you pass
to the_function. If you pass a "void *" there is (essentially) no type
safety. e.g. a stupid example, if the_function() was changed to take an
"int *" above, you'd get a type warning. If you only passed the "void
*", you wouldn't.
This is not really a concern with use of sqlite2, but I think it's a
good idea in general ;)
Regards,
joe
Re: svn commit: r412252 - /apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2006-06-07 at 12:44 +0100, Joe Orton wrote:
> Another useful thing that can be done in general is to do e.g.
>
> static apr_status_t blah(void *data)
> {
> char *foo = data;
> the_function(foo)
>
> since passing a variable of the right type to the function rather than
> just void * gives better type-safety.
Is this something that inherent in C spec, or is it something that some
compilers tend to screw up?
--
Bojan
Re: svn commit: r412252 - /apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jun 07, 2006 at 09:32:23PM +1000, Bojan Smojver wrote:
> On Wed, 2006-06-07 at 12:29 +0100, Joe Orton wrote:
>
> > Yup, exactly what I had in mind ;)
>
> Cool. I have those little functions all over my own code, but I never
> actually thought about their usefulness until now. Well, you learn
> something new every day :-)
Another useful thing that can be done in general is to do e.g.
static apr_status_t blah(void *data)
{
char *foo = data;
the_function(foo)
since passing a variable of the right type to the function rather than
just void * gives better type-safety.
joe
Re: svn commit: r412252 - /apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2006-06-07 at 12:29 +0100, Joe Orton wrote:
> Yup, exactly what I had in mind ;)
Cool. I have those little functions all over my own code, but I never
actually thought about their usefulness until now. Well, you learn
something new every day :-)
--
Bojan
Re: svn commit: r412252 - /apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jun 07, 2006 at 09:25:07PM +1000, Bojan Smojver wrote:
> On Wed, 2006-06-07 at 21:02 +1000, Bojan Smojver wrote:
>
> > I noticed these casts in other cleanup registrations, so I just assumed
> > they were safe. What did you have in mind? A wrapper function?
>
> Like this?
Yup, exactly what I had in mind ;)
Re: svn commit: r412252 - /apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2006-06-07 at 21:02 +1000, Bojan Smojver wrote:
> I noticed these casts in other cleanup registrations, so I just assumed
> they were safe. What did you have in mind? A wrapper function?
Like this?
--
Bojan
Re: svn commit: r412252 - /apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2006-06-07 at 09:56 +0100, Joe Orton wrote:
> This is really still a long way from "properly" - both the old and new
> code has undefined behaviour because of the function cast.
I noticed these casts in other cleanup registrations, so I just assumed
they were safe. What did you have in mind? A wrapper function?
--
Bojan
Re: svn commit: r412252 - /apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jun 07, 2006 at 02:04:44AM -0000, bojan@apache.org wrote:
> Author: bojan
> Date: Tue Jun 6 19:04:43 2006
> New Revision: 412252
>
> URL: http://svn.apache.org/viewvc?rev=412252&view=rev
> Log:
> Properly free results table.
>
> Modified:
> apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
>
> Modified: apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c
> URL: http://svn.apache.org/viewvc/apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c?rev=412252&r1=412251&r2=412252&view=diff
> ==============================================================================
> --- apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c (original)
> +++ apr/apr-util/trunk/dbd/apr_dbd_sqlite2.c Tue Jun 6 19:04:43 2006
> @@ -97,7 +97,7 @@
> (*results)->random = seek;
>
> if (tuples > 0)
> - apr_pool_cleanup_register(pool, *result, (void *) free,
> + apr_pool_cleanup_register(pool, result, (void *) sqlite_free_table,
> apr_pool_cleanup_null);
This is really still a long way from "properly" - both the old and new
code has undefined behaviour because of the function cast.
joe