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: [email protected]
> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> Home Page: http://linux-ha.org/
_______________________________________________________
Linux-HA-Dev: [email protected]
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/
