On 11/01/2012 11:23 PM, ^..^ wrote:
>
> Agree w. Z; you could check for success/fail on that line. A few minor
> additional comments (I
> didn't run it, just looked at it, since I didn't have the patch installed to
> try):
Thanks for the review and suggestions. I have uploaded script v0.7 to the
tracker for review.
I have addressed the gettext.sh check like this. let me know if this looks
fine. I do not like to
use NEWLINE. But could not think of anything better short of a function that
handles all printing to
screen.
if [ -f /bin/gettext.sh ]; then
. gettext.sh
OUTPUT="eval_gettext"
NEWLINE="printf \"\n\""
else
OUTPUT="echo"
NEWLINE=""
fi
${OUTPUT} "${SCRIPT_NAME}: ipmitool(1) not found." 1>&2
...
${NEWLINE}
>
> 1) if the /run directory doesn't exist this will not work, as you don't check
> for its existence,
> only the file /run/bmc-info (is that a agentfreemgmt thing? I searched for it
> and only found Run
> DMC references ;) Generally new top level dirs suck; I'd expect it in
> /var/xyz or some other
> location.)
>
Changed this to '/var/run'. In any case, on Fedora, /var/run -> ../run
> 2) restart should do "stop", then "start", not just "start"
Fixed.
>
> 3) reload shouldn't just do a start. I'm not even sure it's appropriate to
> include or what it
> should do (e.g. you're not sending a signal to a daemon, you're querying the
> BMC.)
>
Only thought behind reload was to update changes in hostname and update any
changes in IP. But that
is the same as start. So a restart is really what we need. I have therefore
dropped reload.
> 4) probably shouldn't assume channel 1 is the right LAN channel; I looked
> aroundÂ
in this note:
>
> http://ingvar.blog.redpill-linpro.com/2011/12/09/setting-the-ip-address-on-hp-ilo-from-linux/
>
> The author searches for the right LAN channel with a little one liner:
>
> for i in `seq 1 14`; do ipmitool lan print $i 2>/dev/null | grep -q ^Set &&
> echo Channel $i; done
>
>
> And then does the LAN print (in their example it was 2). Not sure what the
> most appropriate way
> to find it, butÂ
.
We can cycle through all 14 channels and pick the first one that is active.
>
> 5) Instead of:
>
> eval_gettext "Usage: $0 {start|stop|restart|reload|status}"; echo 1>&2
>
> You probably want/meant:
>
> eval_gettext "Usage: $SCRIPT_NAME {start|stop|restart|status}"
Fixed.
>
> 6) In case of version weirdness you might put in a version check that just
> bails from the start
> (same if the binary doesn't exist; why do anything if it's not there or if
> it's the wrong
> version? You say "failed to communicate with BMC" when the binary doesn't
> existÂ
yes, that's
> true, but not very informative ;)
>
Fixed with more useful error messages.
> 7) In non-dell systems you assume that the URL is "https://($BMC_IPv4):443".
> I don't think you
> should assume a web server is running, let alone that it's on 443 or using
> HTTPSÂ
?
Non Dell systems will not set BMC_URL if they are not coded. And even on Dell
systems, I do not
synthesize URL any more.
>
> 8) you sometimes use "echo" and other times "echo 1>&2". Not that it really
> mattersÂ
but pick
> one (not sure why echo needs the 1>&2, but you never know :))
Sorry. Sticking to 1>&2 everywhere.
>
> 9) (security paranoia on this one, but I can't pass it upÂ
.) You're
> taking unfiltered values
> from the BMC and using them in a shell script on the host OS. This means
> that an attacker can
> set values in BMC land that would affect the server side (backticks and the
> like.) Ensuring that
> the values are kosher (e.g. all chars or #'s or whatever you would expect)
> would be a good
> thing.
I do some sanity checks for IP. URL is on the way.
valid_ip()
{
#Thanks to mkyong.com
octet="([01]?[[:digit:]][[:digit:]]?|2[0-4][[:digit:]]|25[0-5])"
echo ${TMP_IPv4}| grep -Eq "^${octet}\\.${octet}\\.${octet}\\.${octet}$"
return $?
}
valid_ip && BMC_IPv4=${TMP_IPv4} || BMC_IPv4=""
>
> -- d
>
> ^..^
>
> On Nov 1, 2012, at 1:37 AM, Zdenek Styblik <zdenek.styb...@gmail.com> wrote:
>
>> On Mon, Oct 29, 2012 at 9:02 AM, <charles_r...@dell.com> wrote: [...]
>>>
>>> Uploaded both script and config files to Feature Tracker.
>>>
>>> http://sourceforge.net/tracker/?func=detail&aid=3571445&group_id=95200&atid=610553
>>>
>>
>> The only concern I have what happens if somebody doesn't have gettext
>> installed, resp.
>> gettext.sh is not in path. Other than that, I'd say it's ok.
>>
>> Regards, Z.
>>
>> ------------------------------------------------------------------------------
>> Everyone hates
>> slow websites. So do we. Make your web apps faster with AppDynamics Download
>> AppDynamics Lite
>> for free today: http://p.sf.net/sfu/appdyn_sfd2d_oct
>> _______________________________________________ Ipmitool-devel mailing list
>> Ipmitool-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
>
>
> ------------------------------------------------------------------------------
> Everyone hates
> slow websites. So do we. Make your web apps faster with AppDynamics Download
> AppDynamics Lite for
> free today: http://p.sf.net/sfu/appdyn_sfd2d_oct
> _______________________________________________
> Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
>
------------------------------------------------------------------------------
LogMeIn Central: Instant, anywhere, Remote PC access and management.
Stay in control, update software, and manage PCs from one command center
Diagnose problems and improve visibility into emerging IT issues
Automate, monitor and manage. Do more in less time with Central
http://p.sf.net/sfu/logmein12331_d2d
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
