Ovirt devel - Jun 2008 - [PATCH]: Rewrite the ovirt-identify-node in bash

All,
     Attached is a patch to rewrite the ovirt-identify-node script in bash.
Note that this is a drop-in replacement for the current python script; in
particular, it doesn't do any restructuring like Darryl's new code is
going to
do.  However, given that we need to get rid of python because of my last patch,
I think this is a fine interim step.  Note that this patch must be applied on
top of the previous "Shrink Ovirt Node" patch to apply cleanly.

Signed-off-by: Chris Lalancette <clalance at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ovirt-identify-node-bash.patch
Type: text/x-patch
Size: 6435 bytes
Desc: not available
URL:
<http://listman.redhat.com/archives/ovirt-devel/attachments/20080611/b0dba059/attachment.bin>

ACK for 1:1 replacement

just one thing:
> +# gather our information
> +uuid=`hostname -f`
This matches the current behavior, but ideally we'd use HAL here:
hal-get-property --udi /org/freedesktop/Hal/devices/computer --key
system.hardware.uuid

Problem is that hal RPM has lot of deps, so it would not help shrinking effort.
Any ideas about a portable, but w/o HAL, way to get hardware UUID?

Chris Lalancette wrote:
> All,
>      Attached is a patch to rewrite the ovirt-identify-node script in bash.
> Note that this is a drop-in replacement for the current python script; in
> particular, it doesn't do any restructuring like Darryl's new code
is going to
> do.  However, given that we need to get rid of python because of my last
patch,
> I think this is a fine interim step.  Note that this patch must be applied
on
> top of the previous "Shrink Ovirt Node" patch to apply cleanly.
> 
> Signed-off-by: Chris Lalancette <clalance at redhat.com>
Some minor comments below.
> --- a/ovirt-host-creator/common-post.ks	2008-06-11 12:12:03.000000000 +0000
> +++ b/ovirt-host-creator/common-post.ks	2008-06-11 12:12:35.000000000 +0000
> @@ -14,10 +14,10 @@ EOF
[snip]
> +echo_error() {
> +	echo "$@" > /dev/stderr
> +}
I'm a big fan of consistency... In create-wui-appliance and 
ovirt-wui-install we've gone with some of Jim's structure using
functions
like warn, try_h and die.  We should probably do the same here.
> -EOF
> +usage() {
> +	echo_error "Usage: ovirt-identify-node -s <server> -p
<port>"
> +	exit 1
> +}
Same thing for the usage function as above.
> -chmod +x /sbin/ovirt-identify-node
> +send_key_value() {
> +	echo "$1=$2" 1>&3
> +
> +	read 0<&3
> +	if [ "$REPLY" != "ACK $1" ]; then
> +		echo_error "Failed acknowledge of key $1"
> +		exit 4
> +	fi
> +}
> +
> +############# MAIN ##################
> +
> +# parse our options
> +while getopts ":s:p:" flag ; do
> +	case "$flag" in
> +		s)
> +			server=$OPTARG
> +			;;
> +		p)
> +			port=$OPTARG
> +			;;
> +		?)
> +			usage
> +			;;
> +	esac
> +done
> +
> +if [ $(( $# - $OPTIND )) -ge 0 ]; then
> +	usage
> +fi
>  
> +if [ -z "$server" -o -z "$port" ]; then
> +	usage
> +fi
Could use one liners with test here instead of if statements.
> +# gather our information
> +uuid=`hostname -f`
> +arch=`uname -i`
> +memsize=$(( `getconf _PHYS_PAGES` * `getconf PAGESIZE` / 1024 / 1024 ))
> +numcpus=`getconf _NPROCESSORS_ONLN`
> +speed=`grep "cpu MHz" /proc/cpuinfo | uniq | cut -d':'
-f2 | sed -e 's/^[[:space:]]*\(.*\)$/\1/' -e
's/^\(.*\)[[:space:]]*$/\1/'`
> +hostname=`hostname -f`
> +hypervisor="QEMU"
For bundled installs this won't be true, since those will have KVM 
hypervisors.  Can we look at cpuflags to figure out whether to put QEMU or 
KVM here?
> +# FIXME: check that the above values are sane
> +
> +# open our connection to the remote host
> +# FIXME: this is ugly if it fails; redirecting to stderr doesn't seem
to work,
> +# probably because of exec
> +exec 3<> /dev/tcp/$server/$port
> +if [ $? -ne 0 ]; then
> +	echo_error "Connection to $server:$port failed"
> +	exit 5
> +fi
> +
> +# say hello
> +read 0<&3
> +if [ "$REPLY" != "HELLO?" ]; then
> +	echo_error "Expected response HELLO?, received response $REPLY"
> +	exit 2
> +fi
> +echo "HELLO!" 1>&3
> +
> +# OK, start sending our information
> +read 0<&3
> +if [ "$REPLY" != "INFO?" ]; then
> +	echo_error "Expected response INFO?, received response $REPLY"
> +	exit 3
> +fi
> +
> +send_key_value "UUID" "$uuid"
> +send_key_value "ARCH" "$arch"
> +send_key_value "MEMSIZE" "$memsize"
> +send_key_value "NUMCPUS" "$numcpus"
> +send_key_value "CPUSPEED" "$speed"
> +send_key_value "HOSTNAME" "$hostname"
> +send_key_value "HYPERVISOR_TYPE" "$hypervisor"
> +
> +echo "ENDINFO" 1>&3
> +
> +read 0<&3
> +
> +if [ "${REPLY:0:4}" != "KTAB" ]; then
> +	echo_error "Expected response KTAB <filename>, received
response $REPLY"
> +	exit 6
> +fi
> +echo "${REPLY:5}"
> +EOF
> +chmod +x /sbin/ovirt-identify-node
>  
>  echo "Writing ovirt-functions script"
>  # common functions

Chris Lalancette <clalance at redhat.com> wrote:
>      Attached is a patch to rewrite the ovirt-identify-node script in bash.
> Note that this is a drop-in replacement for the current python script; in
> particular, it doesn't do any restructuring like Darryl's new code
is going to
> do.  However, given that we need to get rid of python because of my last
patch,
> I think this is a fine interim step.  Note that this patch must be applied
on
> top of the previous "Shrink Ovirt Node" patch to apply cleanly.
Nice!
only a few nit-picky suggestions...
> +# gather our information
> +uuid=`hostname -f`
> +arch=`uname -i`
> +memsize=$(( `getconf _PHYS_PAGES` * `getconf PAGESIZE` / 1024 / 1024 ))
> +numcpus=`getconf _NPROCESSORS_ONLN`
> +speed=`grep "cpu MHz" /proc/cpuinfo | uniq | cut -d':'
-f2 | sed -e 's/^[[:space:]]*\(.*\)$/\1/' -e
's/^\(.*\)[[:space:]]*$/\1/'`
speed=$(sed -n "/cpu MHz/{s/.*://p;q;}" /proc/cpuinfo|tr -dc 0-9.)

That saves a few pipes, but also avoids the potential for problems
if you have CPUs with differing speeds -- it just takes the first one.

These days (esp with bash, where portability isn't an issue at all),
I prefer to use $(...) over `...`, because you can nest them -- plus
a little more readable, imho.

You have to resort to using `...` only if you're targeting the old
style /bin/sh that are still around, but that no one uses if they can
avoid it.  Even in coreutils tests, which try to be ultra portable,
I've been able to switch, since it uses a cool m4 macro from autoconf
to find a POSIX-conforming (and thus supports $(...) syntax, functions,
etc.) bourne shell that works just about everywhere.

Connecting them all with && you can detect if any fail:

all_ok=0
uuid=$(hostname -f) &&
  arch=$(uname -i) &&
  memsize=$(( $(getconf _PHYS_PAGES) * $(getconf PAGESIZE) / 1024 / 1024 ))
&&
  numcpus=$(getconf _NPROCESSORS_ONLN) &&
  speed=$(sed -n "/cpu MHz/{s/.*://p;q;}" /proc/cpuinfo|tr -dc 0-9.)
&&
    all_ok=1
test $all_ok = 1 || die "something failed; see above"

Chris Lalancette wrote:
> All,
>      Attached is a patch to rewrite the ovirt-identify-node script in bash.
> Note that this is a drop-in replacement for the current python script; in
> particular, it doesn't do any restructuring like Darryl's new code
is going to
> do.  However, given that we need to get rid of python because of my last
patch,
> I think this is a fine interim step.  Note that this patch must be applied
on
> top of the previous "Shrink Ovirt Node" patch to apply cleanly.
> 
> Signed-off-by: Chris Lalancette <clalance at redhat.com>
Committed, with all of the changes suggested by Jim, and all of the changes
suggested by Perry (except for the "hypervisor"; I've left it as
QEMU for now,
and we'll have to use the libvirt capabilities API in the future to get more
detailed information).

Chris Lalancette