[ previous ] [ next ] [ threads ]
 To :  yate@v...
 From :  Paul Chitescu <paulc@v...>
 Subject :  Re: [yate] Patch storm
 Date :  Mon, 14 Jun 2010 17:28:32 +0300
On Monday 14 June 2010 10:26:04 am Allan Sandfeld Jensen wrote:
> On Friday 11 June 2010, Paul Chitescu wrote:
> > Some patches have problems in other usage scenarios than you are using 
> > so we need to modify or reject them. For example the
> > 0007-Use-realloc-in-String- append patch can cause crashes or abnormal
> > results in code like:
> > 
> > String a = "abc"; a += a;
> > 
> Well, there is a theoretical risk because YATE is missing a "String 
> opertor+=(String)" function, and String has the defined the typecast 
> to "const char*". If the you use the operator+ (String, String) instead, 
> that function will take the copy that is needed to make the operation safe.
> Btw, please note that the append operation is done in a fashion that makes 
> read-safe even from different threads. This is very carefully written code 
> (not safe across mulitple threads writing though, but neither are the old 
> String classes).

In this case a += a turns into a.operator+=(a.operator const char*). Other 
similar operations like a += a.c_str()+3 would have the same problem.

While such operation is not currently used in Yate code there are cases where 
the assignment operator is used to cut off from the start of the string like a 
= a.c_str()+3 and one may extend that to the appending operator unaware of its 

Did you check how much improvement would bring this patch after fixing the 
tight loops in the (un)escape methods which I intend to apply?

BTW, I noticed the change in String::safe():

return this ? ...

If "this" is unexpectedly NULL everything is already too screwed for the code 
to continue to run, better crash immediately.

If the intention is to enable coding s->c_str() without checking first if s is 
NULL - please don't do such a thing, use TelEngine::c_safe(s) instead.

> > Other patches my not be portable across all architectures or need
> > alternative implementation - the 0004-Check-mutex-error is part of this
> > cathegory. Mutexes are different in Windows and some functionality like
> > EBUSY is not supported on all *BSD platforms.
> Are you sure? I know I have tried myself, but currently the patch is only 
> useful on non-linux platforms. Linux has the phtread_mutex_timeslock, so 
> code is no longer used there. All the BSDs and even Windows have AFAIK been 
> POSIX certified and EBUSY return from a pthread_mutex_trylock is the central 
> defined baviour of that function. It should work in every implementation, 
> unless the implementation is really broken.

Unfortunately there are really broken implementations of the C library in the 
embedded systems. Not all such systems afford (or politically allow) a 
complete glibc or BSD C library. Just look at the horrible hack implemented by 
s_unsafe, MUTEX_STATIC_UNSAFE and Lockable::startUsingNow().

> > We will apply most of the functionality enhancements, especially those 
> > are not influencing existing installations.
> > 
> > About performance enhancements - although it's obvious from the code that
> > it can be improved in this direction, profiling Yate doesn't show it would
> > benefit a lot from them so we prefer to keep the code easy to understand
> > as much as possible. There performance bottlenecks are pretty obvious and
> > we are fixing them whenever possible. Valgrind is a great tool :-)
> > 
> > Finally, there are some patches introducing quite substantial changes that
> > need more analysis. Stay tuned, we will continue to reply to this thread
> > with the progress and any feedback.
> > 
> Thanks for looking through the patches. I greatly appreciate any feedback 
> will happily answer any questions
> Best regards
> Allan

All best,