Hi Dejan, thanks for the feedback! We've worked in most of your suggested changes, see below:
On 2011-11-10 13:14, Dejan Muhamedagic wrote: > 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/;$//) https://github.com/fghaas/resource-agents/commit/088ba39b855d4ca6375a17500aa0c0e1a2578db8#heartbeat/asterisk > This construct looks a bit unusual: > > if [ ! $? -eq 0 ]; then > > More direct would be: > > if [ $? -ne 0 ]; then https://github.com/fghaas/resource-agents/commit/bbe7a0ba38d366b25067b09141208087a9e44850#heartbeat/asterisk > 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 https://github.com/fghaas/resource-agents/commit/d77afe185d9cec53388c2248ce3b290f95e4cad5 > Line 273 in monitor is going to produce a lot of logging, better > reduce severity to debug: > > ocf_log info "Asterisk PBX monitor succeeded"; https://github.com/fghaas/resource-agents/commit/cf130ce1a3d1b9502ad07df6361f611a256ee560#heartbeat/asterisk > 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 https://github.com/fghaas/resource-agents/commit/80ea432336a56cf2680f30c389b06c20f43eef79#heartbeat/asterisk > Should check content of $pid before line 377 in stop. I'm unsure what you're suggesting here. - Just check that the pid file is non-empty? - Check whether its contents are numeric? - Read the pid and do a kill -0 before kill -TERM? Cheers, Florian -- Need help with High Availability? http://www.hastexo.com/now _______________________________________________________ Linux-HA-Dev: [email protected] http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev Home Page: http://linux-ha.org/
