Hi, On Thu, Nov 10, 2011 at 10:27:36AM +0100, Florian Haas wrote: > On 2011-11-09 12:02, Martin Gerhard Loschwitz wrote: > > Hello everybody, > > > > I wrote an asterisk OCF resource agent which I am hereby putting up > > for discussion. Any feedback is welcome. > > > > It's available from > > https://github.com/fghaas/resource-agents/blob/master/heartbeat/asterisk > > Let's move this thread to the -dev list where it really belongs. > > FWIW, I consider this RA in pretty good shape -- I did review it rather > thoroughly and sent a few patches, for which Martin was kind enough to > include me, undeservingly, in the authors list. Feedback from others > would still be very much appreciated (even if it's just a "+1 for > merge"). Thanks!
Just a few remarks: Ending commands with ';' is not necessary: return $OCF_ERR_INSTALLED; i.e. ';' serves as a command separator. (:%s/;$//) This construct looks a bit unusual: if [ ! $? -eq 0 ]; then More direct would be: if [ $? -ne 0 ]; then Is this necessary (in asterisk_status): if [ -d /proc -a -d /proc/1 ]; then [ "u$pid" != "u" -a -d /proc/$pid ] else ocf_run kill -s 0 $pid fi Why not just: kill -s 0 $pid Line 273 in monitor is going to produce a lot of logging, better reduce severity to debug: ocf_log info "Asterisk PBX monitor succeeded"; In asterisk_start() $ASTRUNDIR is first created using install(8), then again checked in lines 292-296 and created using mkdir, chown, etc. Superfluous. Start may exit with some arbitrary error code (line 324 in asterisk_start()). Perhaps to move all local statements to the top of the function in asterisk_start(). [nitpicking] start_wait is not needed, why not just [ $rc -eq $OCF_SUCCESS ] && break Should check content of $pid before line 377 in stop. Thanks for great work! Cheers, Dejan > Cheers, > Florian > > -- > Need help with High Availability? > http://www.hastexo.com/now > _______________________________________________________ > Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org > http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev > Home Page: http://linux-ha.org/ _______________________________________________________ Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev Home Page: http://linux-ha.org/