[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/#review198
-----------------------------------------------------------



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

    \retval 0 success
    \retval -1 failure



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

    You're missing the AST_REGISTER_FILE_VERSION macro in this file



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

    Why do you need this struct?  Couldn't you simplify this by just putting the LIST_ENTRY field in the calendar tech itself?  If it's only ever going to be linked into a single list, that is the way to go.



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

    same comments for this as I said for caltechlist



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

    You have a memleak of the caltechlist struct here



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

    This will not quite solve the issue.  I suppose this can work, but _only_ if the only time notify_sched will be -1 is when the associated scheduler entry is not currently running.  However, from a quick look, setting it to -1 happens in the middle of the scheduler callback.
    
    I would really rather handle this scheduler stuff outside of the destructor.
    
    Basically, you need a new function that you call when you're releasing a reference to the object when you know that it is time for the object to get destroyed.  In this new function, you will attempt to cancel it's scheduler entries and then release the reference.
    
    At this point, the destructor will either run now, or as soon as any current scheduler entries finish running, assuming that the entries in the scheduler have a reference to the object.
    
    We can talk about this further tomorrow if you would like.



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

    Why did you use 0 here instead of -1 like the other instances of this construct?



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

    I'm still not a big fan of this part.  :-)
    
    It means that configuration loading will only work properly when the technology modules get loaded at startup time.  If you manually load a module from the CLI, the configuration will not get loaded unless you execute a "reload".
    
    Please use the calendar technology registration as a trigger for loading configuration for that technology backend.


- Russell


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
> 
>