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

[Russell Bryant]

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



/trunk/include/asterisk/calendar.h
<http://reviewboard.digium.com/r/58/#comment282>

    Please fill out the rest of the doxygen docs to include return values and arguments



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment283>

    Minor nitpick ... copyright is just 2008



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment284>

    This list needs to be protected by a lock.



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment285>

    It would probably be good to add a comment to make it clear that this line is just for defining a type.



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment286>

    This seems like a good candidate for ao2_callback().  Using the new argument, you can pass a variable that the callback sets to indicate that busy status is found.  If busy status is found, then the callback can also return CMP_STOP to stop the search.  This will result in more efficient operation.



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment287>

    This is a good candidate for an ao2_callback() that uses the options OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE.



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment288>

    There is a race condition here.  If the scheduler entry is already running, you won't be able to delete it.  That means that another thread could be accessing this event while it is being free'd.  Scheduler entries that take an event should take into account their reference count.
    
    Again, the way to handle this is to have another function that you call when you know you want the event to go away.  This function would attempt to cancel scheduler entries and release a reference.  Then, when all references are released, it will get freed.



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment289>

    ao2_callback with OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE with a callback that always returns CMP_MATCH would do the trick here.



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment290>

    Minor thing ... use sizeof(*event)



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment292>

    This should probably be const char *



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment291>

    I can't find any other reference to this flag.  Is anything missing from the diff?
    
    Also, I'm concerned about this blocking.  How is the startup process going to continue if load_config() is blocking and doesn't return?


- Russell


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