[quagga-dev 10433] Re: [PATCH] lib: add skiplists; use for thread sorting by default

Christian Franke chris at opensourcerouting.org
Wed Apr 17 16:37:01 BST 2013


On 04/16/2013 07:20 PM, G. Paul Ziemba wrote:
> chris at opensourcerouting.org (Christian Franke) writes:
>>>    alarm_time.tv_sec = relative_time.tv_sec + time_relative->tv_sec;
>>> +  if (alarm_time.tv_sec < 0)
>>> +    alarm_time.tv_sec = TIME_T_MAX;
>>>    alarm_time.tv_usec = relative_time.tv_usec + time_relative->tv_usec;
>>>    thread->u.sands = timeval_adjust(alarm_time);
> 
>> Can you elaborate on what the purpose of this added code is and when
>> this code path will be taken? My best bet would be that it should run if
>> the addition in the line before the change overflows?
> 
> Yes, this change is meant to address the overflow case.

In that case this feels somewhat moot to me, for some reasons:

a) signed overflow is undefined. GCC may decide to remove that check
alltogether (probably not, as it is hard to determine that the input is
positive), but nevertheless it is not right to trigger an overflow and
check whether it occured, but one should check beforehand or cast to
unsigned (where overflow is defined) etc.
b) this is one check for overflow in a sea of many additions without any
check. E.g. the timeval_adjust(alarm_time) will add one to
alarm_time.tv_sec if alarm_time.tv_usec is >= 1000000 which is quite
likely to happen.
c) When exactly does that happen? If I am not mistaken, a timer set to
expire at TIME_T_MAX is not likely to run anytime soon. When do we
schedule something to run "never"?

Don't get me wrong, if there is an issue with overflows, we should
definitively work on fixing it. However I am not yet certain that there
really is a problem that would be correctly solved by that addition.

>>> +#ifdef THREAD_TIMER_USE_SKIPLIST
>>> +  void  *skiplist          = NULL;
>>> +#endif
>> ...
>>> +  void *skiplist_timer;              /* use instead of timer thread_list */
>>> +  void *skiplist_background;
> 
>> Why do you pass around skiplists using void* instead of struct skiplist*?
> 
> I think (trying to remember code I wrote two years ago) I was trying to
> avoid declaring the pointers in struct thread_master as explicit skiplist
> type, in turn to avoid avoid requiring inclusion of skiplist.h everywhere
> that thread.h was included.
> 
> Maybe a better approach would be to just #include <skiplist.h> in
> thread.h and declare the pointers in struct thread_master as
> skiplist type. I'm open to suggestions.

I added a forward declaration "struct skiplist;" to lib/thread.h and
added "struct skiplist *skiplist_timer" to the thread master structure.
This seems to work fine.

-Christian




More information about the Quagga-dev mailing list