[PATCH] svnadmin lsdblogs, fs.c, Windows

[Martin Hauner <ha...@web.de>]

Hi all,

here is a small fix for a memory problem in svn_fs_berkeley_logfiles.
It gets called when you run svnadmin lsdblogs.

attached is an 'in place fix'...

-- 
Martin

Re: [PATCH] svnadmin lsdblogs, fs.c, Windows

[Martin Hauner <ha...@web.de>]

Russell Yanofsky wrote:

> Martin Hauner wrote:
> 
>>Russell Yanofsky wrote:
>>
>>
>>>>+  SVN_BDB_ERR (env->set_alloc (env,malloc,realloc,free));
>>>>...
>>>>-  free (filelist);
>>>>+  env->db_free (filelist);
>>>
>>>
>>>Why add a call to set_alloc AND switch from free to db_free? Either
>>>change should be sufficient by itself.
>>
>>Sure, but I think env->db_free() better communicates that filelist
>>came from db.
> 
> 
> I think so too. But if you use it can't you get rid of the set_alloc call?

Ah, now I see what you were talking about in your first reply. :)
The problem here is that db_env_create doesn't set the memory stuff
itself. So without set_alloc db_free is NULL.


-- 
Martin





---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] svnadmin lsdblogs, fs.c, Windows

[Russell Yanofsky <re...@columbia.edu>]

Martin Hauner wrote:
> Russell Yanofsky wrote:
> 
>>> +  SVN_BDB_ERR (env->set_alloc (env,malloc,realloc,free));
>>> ...
>>> -  free (filelist);
>>> +  env->db_free (filelist);
>> 
>> 
>> Why add a call to set_alloc AND switch from free to db_free? Either
>> change should be sufficient by itself.
> 
> Sure, but I think env->db_free() better communicates that filelist
> came from db.

I think so too. But if you use it can't you get rid of the set_alloc call?

- Russ


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] svnadmin lsdblogs, fs.c, Windows

[Martin Hauner <ha...@web.de>]

Russell Yanofsky wrote:

>>+  SVN_BDB_ERR (env->set_alloc (env,malloc,realloc,free));
>>...
>>-  free (filelist);
>>+  env->db_free (filelist);
> 
> 
> Why add a call to set_alloc AND switch from free to db_free? Either change
> should be sufficient by itself.

Sure, but I think env->db_free() better communicates that filelist
came from db.

-- 
Martin


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] svnadmin lsdblogs, fs.c, Windows

[Russell Yanofsky <re...@columbia.edu>]

Martin Hauner wrote:
> ...
> +  /* Windows: tell the bdb dll to use "our" heap to allocate filelist.
> +     On Windows dlls have their own heap and we have to make sure
> +     filelist is allocated from the same heap we free it from. */

Minor correction: "dlls have their own heap" should be "dlls sometimes have
their own heaps".

> +  SVN_BDB_ERR (env->set_alloc (env,malloc,realloc,free));
> ...
> -  free (filelist);
> +  env->db_free (filelist);

Why add a call to set_alloc AND switch from free to db_free? Either change
should be sufficient by itself.

- Russ



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] svnadmin lsdblogs, fs.c, Windows

[cm...@collab.net]

Martin Hauner <ha...@web.de> writes:

> Hi all,
> 
> here is a small fix for a memory problem in svn_fs_berkeley_logfiles.
> It gets called when you run svnadmin lsdblogs.
> 
> attached is an 'in place fix'...

I'll try to review this when next I'm near a Windows box, unless
someone else gets to it first (Brane?).

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] svnadmin lsdblogs, fs.c, Windows

[Russell Yanofsky <re...@columbia.edu>]

cmpilato@collab.net wrote:
> ...
> I'll commit up the set_alloc portion of the change so that this code
> will be consistent with the rest of the Subversion code that uses
> Berkeley environments.

The change you committed should fix the problem. Calling env->db_free() is
pretty much the same as calling free() since env->db_free is set to a
pointer to &free by set_alloc.

- Russ



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] svnadmin lsdblogs, fs.c, Windows

[cm...@collab.net]

Martin Hauner <ha...@web.de> writes:

> Hi all,
> 
> here is a small fix for a memory problem in svn_fs_berkeley_logfiles.
> It gets called when you run svnadmin lsdblogs.
> 
> attached is an 'in place fix'...
> 
> -- 
> Martin
> * subversion/libsvn_fs/fs.c
>   (svn_fs_berkeley_logfiles): fixed memory alloc/free of filelist on
>   windows.

What's the status of this patch?  Russell seems to think that we don't
need both the set_alloc and db_free.  I'm confused as to why we need
either -- I mean, why is this failing for log file listings, but not
for the dozens of other places in the code (names, all the uses of
DBT's with the DB_DBT_MALLOC flag) where this same type of memory
transfer is happening?  Everything else just uses DB_ENV->set_alloc()
and free().  In fact, I can't even seem to find documentation for
DB_ENV->db_free() on Sleepycat's website!

I'll commit up the set_alloc portion of the change so that this code
will be consistent with the rest of the Subversion code that uses
Berkeley environments.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org