[asterisk-dev] [Code Review] Calendaring API for Asterisk

Terry Wilson twilson at digium.com
Tue Dec 30 18:37:20 CST 2008 


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/main/calendar.c, lines 1400-1408
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1853#file1853line1400>
> >
> >     You have a race condition here.
> >     
> >     It is possible to end up sleeping longer that you really want to.  Here's how it could go down:
> >     
> >     1) You determine how long you should sleep.
> >     
> >     2) Something else gets scheduled that is sooner than what you got from #1
> >     
> >     3) You lock and then go to sleep.
> >     
> >     What you need to do is hold the lock before you determine how long you should sleep.  You will also need to hold the same lock when you add scheduler entries, and also signal the condition when you add entries.
> >     
> >     That will avoid this issue.

done


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/res/res_caldav.c, line 4
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1856#file1856line4>
> >
> >     Update copyright for just 2008

Went ahead and made it 2008 - 2009 since it is almost here


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/res/res_caldav.c, lines 538-540
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1856#file1856line538>
> >
> >     You missed unlocking the calendar here and in a few other error handling blocks

doh.  fixed here and in res_exchangecal and res_icalendar


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/res/res_caldav.c, line 599
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1856#file1856line599>
> >
> >     space after for

done


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/res/res_exchangecal.c, line 86
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1857#file1857line86>
> >
> >     Use ast_strdup() and check for failure

I was leaking this anyway, so I just made it a static buffer.


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/res/res_exchangecal.c, line 136
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1857#file1857line136>
> >
> >     space after for

done


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/res/res_exchangecal.c, line 184
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1857#file1857line184>
> >
> >     space after if

done


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/res/res_exchangecal.c, line 277
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1857#file1857line277>
> >
> >     It looks like this cast isn't needed.  Just declare tmp as a const char *

done


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/res/res_exchangecal.c, lines 507-517
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1857#file1857line507>
> >
> >     coding guidelines :)

done (but I still think this is more readable--maybe we could change the coding guidelines to allow single line ifs for short statements as opposed to making them take 3 lines each at minimum)


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/res/res_exchangecal.c, lines 615-617
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1857#file1857line615>
> >
> >     Missed unlock of the calendar here and in some other error handling blocks

done


> On 2008-12-11 22:16:42, Russell Bryant wrote:
> > /trunk/res/res_exchangecal.c, line 316
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1857#file1857line316>
> >
> >     You have a minor bug here.  With strncpy(), you need to subtract 1 from the size of the buffer.  However, just use ast_copy_string() instead.

done


- Terry


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/58/#review202
-----------------------------------------------------------


On 2008-12-11 10:40:03, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/58/
> -----------------------------------------------------------
> 
> (Updated 2008-12-11 10:40:03)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Calendaring integration for Asterisk--currently supporting iCalendar files (hosted over HTTP/HTTPS), CalDAV, and Microsoft Exchange.  Features available: dialplan functions for querying and writing (if the calendar technology supports it like CalDAV and Exchange do) calendar event information, a device state provider for phone BLF support, and calendar event notification through executing dialplan apps or context/exten.
> 
> The overall structure is that the res_* calendar modules register as a calendar tech to main/calendar.c, and after all modules are loaded, main/calendar.c parses calendar.conf and calls the calendar_load callback for each register calendar tech based on the "type" field for that calendar in calendars.conf.  Currently each calendar runs in its own thread downloading and refreshing the remote calendar data as necessary as to avoid serializing downloads.  
> 
> Eventually it would probably be a good idea for me to implement a thread pool and honor a maximum number of downloads per host:port as well as a global maximum number of simultaneous downloads--perhaps with a min/max refresh value so I could randomize the time for refreshing a bit so as not to be refreshing all of the calendars at once.  Also, there was a request to add support for the dialplan functions to query calendars that aren't in calendar.conf i.e. CALENDAR_QUERY(ical/http://www.google.com/calendar/ical/nkt5atdq7cdbes3ehdfpendpnc%40group.calendar.google.com/public/basic.ics,${EPOCH},$[${EPOCH} + 3600]), etc.
> 
> Currently I don't have any support for querying/setting attendees, mostly because it is a list of results whereas all of the others are individual fields.  Writing gets especially ugly for them because of the current format of CALENDAR_WRITE(calendar,${HASHKEYS(calendar)})=${HASH(calendar)}.  I suppose I could add a ...,${HAHSKEYS(attendees)})=...,${HASH(attendees)} to the end...it is just getting a little ugly.
> 
> 
> Diffs
> -----
> 
>   /trunk/build_tools/menuselect-deps.in 163077 
>   /trunk/configs/calendar.conf.sample PRE-CREATION 
>   /trunk/configure.ac 163077 
>   /trunk/include/asterisk/_private.h 163077 
>   /trunk/include/asterisk/autoconfig.h.in 163077 
>   /trunk/include/asterisk/calendar.h PRE-CREATION 
>   /trunk/main/Makefile 163077 
>   /trunk/main/asterisk.c 163077 
>   /trunk/main/calendar.c PRE-CREATION 
>   /trunk/main/loader.c 163077 
>   /trunk/makeopts.in 163077 
>   /trunk/res/res_caldav.c PRE-CREATION 
>   /trunk/res/res_exchangecal.c PRE-CREATION 
>   /trunk/res/res_icalendar.c PRE-CREATION 
> 
> Diff: http://reviewboard.digium.com/r/58/diff
> 
> 
> Testing
> -------
> 
> I have tested all three calendar modules by adding, deleting, and moving events and verifying that notifications occur.  I have tested writing to Exchange, and Zimbra and Google Calendar through CalDAV.  I have run with MALOC_DEBUG looking for leaks and dropped references to astobj2 objects. 
> 
> 
> Thanks,
> 
> Terry
> 
>

More information about the asterisk-dev mailing list