On Thu, 6 May 2004, Seshubabu Pasam wrote:

> Here is the patch for issue #33. Here is what I did:

Thanks! Here follow a few comments I have on this patch:

> 1.) Added --enable-memcallback flag. This has no effect when --enable-debug
> is used.

I think this code should be unconditionally used, but this approach certainly
makes it easy to enable/disable this during the initial test period.

> 2.) Added curl_set_mem_callbacks function in curl.h when CURL_MEMCALLBACK
> CPPFLAG is set (will be set, if compiled with --enable-memcallback) This
> function can be called with user defined memory management routines.

The API needs to be present at all times. We don't want conditionally present
functions in libcurl.

> 3.) Replaced malloc/free/realloc/strdup with
> curl_malloc/curl_free/curl_realloc/curl_strdup respectively in all .c files
> in lib and src folders.

I think I would rather see that you keep the names malloc/free/realloc/strdup
etc in all the code. Not only are the commonly used and understood, but in the
future when people provide patches it is much simplier and easier if we
continue to use the "standard" function names. Also, not having them named
like that increases the risk that such ones "sneak in" in the future.

We could instead just #define those names to use the proper global function
pointers instead, very similar to how the memory debug system already works.

It would also make your patch MUCH simpler.

> 4.) url.c has 2 'free' calls that are not changed, since the memory is
> allocated externally in libidn.

Good, we must make sure that we maintain this ability.

> 5.) Replaced all calloc's with curl_malloc and memset.

I don't like that. We just recently introduced calloc() after years of not
having it, and I think that it is useful. Again, for conveniance to
developers. I'd rather let the app that provides the callbacks do that
malloc()-then-memset() logic.

> 9.) I am not sure if this is problematic: I removed "curl_free" function
> from curl.h. I did not understand why it is there in the first place. It is
> just a wrapper around 'free'. It is not consistently used in the curl code.
> I am not sure, what functionality it is supposed to provide to the libCurl
> library users. Let me know if this is an issue, I will add it back and
> rename my curl_free macro to something else....

It is problematic because curl_free() is useful and fills a purpose. It
doesn't have to be used within libcurl so there's no inconsistancy there to
worry about.

The whole point of curl_free() is to allow applications to free memory
allocated by libcurl. By providing curl_free() the app makes sure that the
memory is freed with/by the same memory subsystem that allocated the memory.
There are two notable use cases for this:

1. In Windows, you can link the lib and the app with two different memory
   subsystems that makes it impossible for the app to free memory that the
   lib allocated, unless this function is provided.

2. When we build libcurl with memory debugging enabled, we add data around
   each malloced data chunk that needs to be taken care of when the data is
   freed. By providing curl_free(), any ordinary libcurl-using app can
   link/use a libcurl that is built with memory-debugging without having to
   care the least.

--
     Daniel Stenberg -- http://curl.haxx.se -- http://daniel.haxx.se
      Dedicated custom curl help for hire: http://haxx.se/curl.html