[Ovirt-devel] readability/maintainability .ks tweaks

[Jim Meyering]

Alan Pevec <apevec at redhat.com> wrote:
> Jim Meyering wrote:
>> 	readability/maintainability .ks tweaks
>> 	* ovirt-host-creator/common-post.ks: Remove an unnecessary use of xargs.
>> 	Double-quote some $var-containing sed arguments -- just in case.
>> 	Shorten and split some long lines.
>> 	Use printf rather than echo -e to ease readability/maintainability
>> 	with long, \n-separated strings.
>
> ack, thanks!
> Please check-in and I'll repost "get host networking configuration from ovirt server" to include your fixes.

Hi Alan,

Thanks for the quick review.
Pushed.

>> -            SWAPDEVS="$SWAPDEVS `/sbin/fdisk -l $dev 2>/dev/null | tr '*' ' ' | awk '$5 ~ /82/ {print $1}' | xargs`"
>> +            SWAPDEVS="$SWAPDEVS `/sbin/fdisk -l $dev 2>/dev/null | tr '*' ' ' \
>> +	                         | awk '$5 ~ /82/ {print $1}'`"
>
> or
> +            SWAPDEVS="$SWAPDEVS `sfdisk -d $dev 2>/dev/null \ +
> | awk 'substr($0,49,2) == "82" { print $1}'

sfdisk might well be better (I don't know), but we can't use column
offsets like that.  For example, on one of my systems, "49" doesn't work
because the ID starts at column 51 -- it's sensitive to the width of
preceding columns e.g., the maximum block count.

> or just delegate swap probing to udev? e.g.
> --- a/ovirt-host-creator/common-post.ks
> +++ b/ovirt-host-creator/common-post.ks
> @@ -65,14 +65,11 @@ start() {
>         LVMDEVS="$DEVICES `/usr/sbin/lvscan | awk '{print $2}' | tr -d \"'\"`"
>
>        SWAPDEVS="$LVMDEVS"
> -        for dev in $BLOCKDEVS; do
> -            SWAPDEVS="$SWAPDEVS `/sbin/fdisk -l $dev 2>/dev/null | tr '*' ' ' | awk '$5 ~ /82/ {print $1}' | xargs`"
> -        done
>
>        # now check if any of these partitions are swap, and activate if so
>         for device in $SWAPDEVS; do
> -            sig=`dd if=$device bs=1 count=10 skip=$(( $PAGESIZE - 10 )) 2>/dev/null`
> -            if [ "$sig" = "SWAPSPACE2" ]; then
> +            eval `/lib/udev/vol_id $device 2>/dev/null`
> +            if [ "$ID_FS_TYPE" = "swap" ]; then
>                 swapon $device
>             fi
>         done

Oh, nice!  I haven't used /lib/udev/vol_id enough yet ;-)
I much prefer that.  I assume udev is reliable enough.

To be on the safe side:
  - quote the string you eval
  - test $ID_FS_TYPE only if eval succeeds;
      otherwise, you could test a value from a preceding device
  - quote the device name
  - I prefer $(...) to `...` because you can nest the former
  - I have a small preference for "test expr && ..." over
    "if [expr]; then ... fi" partly because it's shorter and
    there's less syntax (i.e. less to type and less to get wrong ;-)

ID_FS_TYPE=
id_fs=$(/lib/udev/vol_id "$device" 2>/dev/null)	\
  && eval "$id_fs"				\
  && test "$ID_FS_TYPE" = swap			\
  && swapon "$device"

[caveat: untested ;-) ]