Re: [Evolution-hackers] Removing libical fork, moving to new upstream?

2008-09-09 Thread Chenthill


On Mon, 2008-09-08 at 23:55 -0400, IGnatius T Foobar wrote:
 Patrick Ohly wrote:
  In the upstream libical certain functions return char * pointers into
  memory stored in ring buffers. The caller must not free those pointers.
  The drawback is that the life time of those strings is not predictable.
 
  In the current Evolution libical, those same functions (not renamed!)
  return copies of the string which the caller has to free. Code which was
  written using the old semantic of the calls will leak memory. Code
  adapted to the new semantic (like Evolution) will crash when combined
  with the upstream libical without the same patch.

 Ok, I definitely see the benefit there.   This is similar to POSIX calls 
 which now offer alternative versions (usually with _r appended to the 
 name) that don't use a static buffer or a ring buffer, in order to be 
 reentrant?
  If all users of the upstream libical are willing to adapt their code,
  then the best solution would be to simply import the Evolution patch
  into upstream.

 As much as I'd like to see that happen, I don't think it's realistic.  
 libical is used by dozens of downstream projects, and a sudden forced 
 API change is likely to encourage them to fork (or stay forked, if 
 they've already done so) -- exactly the opposite of the end we are 
 trying to achieve!
  If there is resistance against that, then we could provide two versions
  of each of these API calls: one with the old name and old behavior and
  one with the new behavior and a name suffix. 
 That seems more realistic.  The alternative might be to offer a global 
 flag that tells libical to behave one way or the other?  (I think 
 something like that was suggested at some point.)
 
 While I definitely think the new method of memory allocation makes far 
 more sense (we'll definitely use it in Citadel, as all of our code is 
 multithreaded) -- expecting the entire community to perform a flag day 
 API change in lockstep is likely to cause confusion and delay.   If we 
 pursued either the alternate function names or the global flag, is there 
 likely to be any pushback from the Evolution team?
I do not feel having alternate function names would be a better
solution.

Consider the following API which remains the same before and after the
memory fixes,
char * icalcomponent_as_ical_string (icalcomponent *icomp);

The returned memory from this API was internally handled by libical
before and now its given to the caller. Though the return type gives an
indication that the memory is owned by caller, it was not the case. So
having a new API for this and changing the behavior does not look to be
a good solution since the underlying memory allocation had to be
changed.

Similarly even with other APIs which return const char* values, the
memory can be overwritten at any time. While removing the ring buffer
return type's of all the APIs had to be changed from const char * to
char *. Is it really worth it to have the old unstable APIs which can
crash the application randomly ? My answer would be NO.

This is not just a problem with multi-threaded programs. The crash could
happen once the ring buffer gets full and starts overwriting the used
memory.

Since we statically link to libical and expose it via libecal, we have
updated the library versions of libecal. We have an additional flag
check (hack) also for it now with a warning as in
http://bugzilla.gnome.org/show_bug.cgi?id=528986 .


So it is better to inform all the stake holders about the change and let
them depend on the library versions to decide whether to free the memory
or not if they have a need to depend on the older versions of libical. I
think no one deny to make the necessary changes knowing that the old API
is not very stable.

Atleast once I noticed the problem. I made this patch and made all the
changes required in evolution, evolution-exchange and
evolution-data-server. I would not really like to change them again with
new APIS :)


thanks, Chenthill.

___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] Removing libical fork, moving to new upstream?

2008-09-09 Thread Patrick Ohly
On Tue, 2008-09-09 at 09:12 -0400, IGnatius T Foobar wrote:
 Chenthill wrote:
  So it is better to inform all the stake holders about the change and let
  them depend on the library versions to decide whether to free the memory
  or not if they have a need to depend on the older versions of libical. I
  think no one deny to make the necessary changes knowing that the old API
  is not very stable.
 
  Atleast once I noticed the problem. I made this patch and made all the
  changes required in evolution, evolution-exchange and
  evolution-data-server. I would not really like to change them again with
  new APIS :)
 Although I agree that the new memory model is a vast improvement over 
 the existing one, I think you may be underestimating the potential 
 effects of telling dozens of downstream projects that they will have to 
 rewrite their code *right* *now* in order to continue using libical.  
 Many will respond by forking, or staying forked, which as I mentioned 
 earlier is exactly the opposite of what we are trying to accomplish.

I agree.

 How would you feel about a global flag which tells libical how to 
 allocate memory?

The problem with that is that it is not possible to mix code which uses
the old and the new semantic. For example, a program or library which
uses libical directly, using the old semantic, couldn't be combined with
the Evolution libraries.

To let old and new code coexists I would suggest the following approach:
 1. The Evolution patch gets applied, making the core functions safe
to use.
 2. The function implementations whose semantic has changed get
renamed; I kind of like the _r suffix, but _alloc or _copy would
also work.
 3. Under the old names small wrappers are added which establish the
old behavior again by copying the string into the ring buffer
and freeing the dynamically allocated one: this incurs some
overhead, but usage of these versions of the calls is
discouraged anyway. By using function attributes it would be
possible to trigger deprecation warnings for code which still
uses them.
 4. In the header file both variants are declared.
 5. In addition, ical.h also redefines the old names to the new
names if the HANDLE_LIBICAL_MEMORY define is set: Evolution code
already does that and therefore continues to work with such a
modified upstream libical without source code changes.

I personally would prefer to avoid step 5 and rather do a search/replace
in Evolution, but Chenthill didn't like that.

-- 
Bye, Patrick Ohly
--  
[EMAIL PROTECTED]
http://www.estamos.de/

___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers


[Evolution-hackers] compile problems with trunk: dolt

2008-09-09 Thread Patrick Ohly
Hello,

I've mentioned it before in the context of MAPI, but let me raise
awareness of it again: dolt uses bash syntax which is not supported by
bash versions still in use today. I mailed the author, but didn't get a
reply. Please consider replacing the += in all acinclude.m4 files
(gtkhtml/evolution/evolution-data-server), otherwise users on older
systems will not be able to compile - I'm one of them. Patch attached,
can file a bug report if that's necessary.

-- 
Bye, Patrick Ohly
--  
[EMAIL PROTECTED]
http://www.estamos.de/
Index: evolution-data-server/acinclude.m4
===
--- evolution-data-server/acinclude.m4	(revision 9502)
+++ evolution-data-server/acinclude.m4	(working copy)
@@ -445,7 +445,7 @@
 case $arg in
 --mode=compile) modeok=true ;;
 --tag=CC|--tag=CXX) tagok=true ;;
-*) args+=($arg)
+*) [EMAIL PROTECTED]$arg ;;
 esac
 done
 if $modeok  $tagok ; then
___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers