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

Tue Mar 17 00:06:05 CDT 2009

> On 2009-01-25 18:25:16, Russell Bryant wrote:
> > /trunk/res/res_caldav.c, lines 639-642
> > <http://reviewboard.digium.com/r/58/diff/6/?file=2272#file2272line639>
> >
> >     Does this module actually export any symbols?  I don't think it does ..

Fixed here and in the other res modules

> On 2009-01-25 18:25:16, Russell Bryant wrote:
> > /trunk/main/calendar.c, lines 1453-1455
> > <http://reviewboard.digium.com/r/58/diff/6/?file=2269#file2269line1453>
> >
> >     return -1 ?

Well, I left it so that it would at least have partial utility if there was an error.  You won't get notifications or refresh the calendar, but you would get the calendars loaded, and if you specify an interval of days/months/years to pull back the first time and you just want to be able to query, you don't absolutely have to have the refresh thread.  So, I left it this way.  If you think that not loading is better, I don't feel strongly one way or the other.

> On 2009-01-25 18:25:16, Russell Bryant wrote:
> > /trunk/main/calendar.c, line 1459
> > <http://reviewboard.digium.com/r/58/diff/6/?file=2269#file2269line1459>
> >
> >     I can see that you changed the config loading.  However, it looks like you missed adding the part where you need to force reloading the configuration when a calendar technology gets loaded.  Or, am I missing something about how this works now?

It has been a while since I looked at it, but ast_calendar_register is called by the tech modules, which then returns with the value of load_tech_calendars() which handles all of the calendar building, etc.

> On 2009-01-25 18:25:16, Russell Bryant wrote:
> > /trunk/res/res_exchangecal.c, lines 84-88
> > <http://reviewboard.digium.com/r/58/diff/6/?file=2273#file2273line84>
> >
> >     This will crash if tmp is NULL.
> >     
> >     If it's not possible for tmp to be NULL, then there is an unnecessary check here.

fixed

> On 2009-01-25 18:25:16, Russell Bryant wrote:
> > /trunk/main/calendar.c, lines 1438-1455
> > <http://reviewboard.digium.com/r/58/diff/6/?file=2269#file2269line1438>
> >
> >     Generally, you want to initialize all data before you register interfaces to the Asterisk core.  Otherwise, you have a small chance that one of the interfaces gets used too soon and accessing something that has not yet been initialized.

done

> On 2009-01-25 18:25:16, Russell Bryant wrote:
> > /trunk/main/calendar.c, lines 1385-1405
> > <http://reviewboard.digium.com/r/58/diff/6/?file=2269#file2269line1385>
> >
> >     It's technically possible that this function could get called by 2 different threads at the same time, which could lead you deleting more data that you really wanted to if timed just right.
> >     
> >     A quick and easy solution to this is to create a reload lock to ensure that only 1 reload is being processed at a time.

done.

> On 2009-01-25 18:25:16, Russell Bryant wrote:
> > /trunk/main/calendar.c, lines 762-765
> > <http://reviewboard.digium.com/r/58/diff/6/?file=2269#file2269line762>
> >
> >     We should convert this over to xml docs

And done.

- Terry

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

On 2008-12-31 17:30:37, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/58/
> -----------------------------------------------------------
> 
> (Updated 2008-12-31 17:30:37)
> 
> 
> 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 167054 
>   /trunk/configs/calendar.conf.sample PRE-CREATION 
>   /trunk/configure.ac 167054 
>   /trunk/include/asterisk/_private.h 167054 
>   /trunk/include/asterisk/autoconfig.h.in 167054 
>   /trunk/include/asterisk/calendar.h PRE-CREATION 
>   /trunk/main/Makefile 167054 
>   /trunk/main/asterisk.c 167054 
>   /trunk/main/calendar.c PRE-CREATION 
>   /trunk/main/loader.c 167054 
>   /trunk/makeopts.in 167054 
>   /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
> 
>