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

Terry Wilson twilson at digium.com
Tue Dec 30 17:18:57 CST 2008 


> On 2008-12-11 17:00:48, Russell Bryant wrote:
> > /trunk/include/asterisk/calendar.h, line 134
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1850#file1850line134>
> >
> >     \retval 0 success
> >     \retval -1 failure

done


> On 2008-12-11 17:00:48, Russell Bryant wrote:
> > /trunk/main/calendar.c, line 22
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1853#file1853line22>
> >
> >     You're missing the AST_REGISTER_FILE_VERSION macro in this file

done


> On 2008-12-11 17:00:48, Russell Bryant wrote:
> > /trunk/main/calendar.c, lines 65-68
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1853#file1853line65>
> >
> >     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.

Good call.  I was just copying from the channel list stuff since this seemed similar.  Didn't really think about it much, honestly.  I guess it is done so that you can define the techs themselves const when you are declaring them in the res_ modules, and ast_calendar_register/unregister would be modifying the tech if the LIST_ENTRY was inside the ast_calendar_tech struct.  But, since there is no real need for them to be const, I went ahead an moved the LIST_ENTRY.


> On 2008-12-11 17:00:48, Russell Bryant wrote:
> > /trunk/main/calendar.c, lines 70-73
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1853#file1853line70>
> >
> >     same comments for this as I said for caltechlist

This one is a little bit different.  Events normally are stored in ao2_containers and not lists.  The only time they are stored in a list is for the purposes of returning the results in a particular order from a CALAENDAR_QUERY call.  When the eventlist's refcount goes to zero (which happens when the channel datastore it is attached to goes away), the destructor removes all entries from the list, but doesn't do anything with the actual event--so I think these should be separate.


> On 2008-12-11 17:00:48, Russell Bryant wrote:
> > /trunk/main/calendar.c, lines 248-250
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1853#file1853line248>
> >
> >     You have a memleak of the caltechlist struct here

I have removed the caltechlist struct entirely now.


> On 2008-12-11 17:00:48, Russell Bryant wrote:
> > /trunk/main/calendar.c, lines 282-286
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1853#file1853line282>
> >
> >     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.

I have changed how this works...hopefully I have it now.


> On 2008-12-11 17:00:48, Russell Bryant wrote:
> > /trunk/main/calendar.c, line 298
> > <http://reviewboard.digium.com/r/58/diff/5/?file=1853#file1853line298>
> >
> >     Why did you use 0 here instead of -1 like the other instances of this construct?

This line no longer exists here after trying to fix the other sched issues.


- Terry


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


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