[Ovirt-devel] Rebased - [PATCH] get host networking configuration from ovirt server

[Jim Meyering]

Alan Pevec <apevec at redhat.com> wrote:
> commit cddbecff92d0d1af74a281bfd353f9faac89e9c1
> Author: Alan Pevec <apevec at redhat.com>
> Date:   Tue May 6 23:15:41 2008 +0200
>
>    get host networking configuration from oVirt server
>     PXE-boot interface[1] is temporarily setup early in the boot
> sequence (init.d/ovirt-early) and configuration[2] is retrieved from
> oVirt server.
>    hostname is put in the requested URL to allow per host configuration, but for now all hosts get the same config: eth0 bridged to ovirtbr0 bridge
>     [1] IPAPPEND 2 in pxelinux config appends MAC of the PXE-booted
> NIC to the kernel cmdln e.g. BOOTIF=01-00-16-3e-12-34-57
>    [2] current implementation is a list of augtool commands, see http://augeas.net/tour.html

Hi Alan,

Looks good.  Though something (your mail client?) mangled the patch,
removing leading spaces -- that makes it so it doesn't apply.
Here are a few comments:

> diff --git a/ovirt-host-creator/common-post.ks b/ovirt-host-creator/common-post.ks
> index 17291b4..6c503b4 100644
> --- a/ovirt-host-creator/common-post.ks
> +++ b/ovirt-host-creator/common-post.ks
> @@ -42,19 +42,60 @@ cat > /etc/init.d/ovirt-early << \EOF
>
> # Source functions library
> . /etc/init.d/functions
> +. /etc/init.d/ovirt-functions
>
> -start() {
> +configure_from_network() {
> +    DEVICE=$1
> +    if [ $DEVICE ]; then

If $DEVICE may somehow expand to a "test" option, like -z, -f, -lt, etc.
then the above will malfunction.  Add quotes and use -n to be safe:

       if [ -n "$DEVICE" ]; then

> +        echo -n "."

No need for double quotes.
Prefer printf (besides, it's one byte shorter ;-)

           printf .

> +        # setup temporary interface to retrieve configuration
> +        echo "network --device $DEVICE --bootproto dhcp" | nash
> +        if [ $? -eq 0 ]; then
> +            echo -n "."
> +            # from network-scripts/ifup-post
> +            IPADDR=$(LANG=C ip -o -4 addr ls dev ${DEVICE} | awk '{ print $4 ; exit }')

I suppose this sets LANG=C to avoid internationalized output from "ip"?
LC_ALL=C is preferable for that, since it trumps LANG.

> +            eval $(ipcalc --silent --hostname ${IPADDR} ; echo "status=$?")
> +            if [ "$status" = "0" ]; then
> +                hostname $HOSTNAME
> +                # retrieve remote config
> +                find_srv ovirt tcp
> +                echo -n "."
> +                if [ $SRV_HOST -a $SRV_PORT ]; then
> +                    curl -s http://$SRV_HOST:$SRV_PORT/ovirt/cfgdb/$(hostname) \

Add double quotes and -n, just in case $SRV_HOST and $SRV_PORT might
ever be bogus (so reviewers can be lazy and not have to ensure --
or assume -- that find_srv or whatever else sets those variables does
the right thing:

                   if [ -n "$SRV_HOST" -a -n "$SRV_PORT" ]; then
                       curl -s "http://$SRV_HOST:$SRV_PORT/ovirt/cfgdb/$(hostname)" \

> +                        | augtool > /dev/null 2>&1
...
> +start() {
> +        # find boot interface from cmdline
> +        # IPAPPEND 2 in pxelinux.cfg appends e.g. BOOTIF=01-00-16-3e-12-34-57
> +        BOOTIF=
> +        for i in $(cat /proc/cmdline); do
> +            case $i in
> +                BOOTIF=*)
> +                    BOOTMAC=$(echo $i | cut -d- -f2- | sed 's/-/:/g')

this could be a little more strict on the input:
                   BOOTIF=??-??-??-??-??-??-??)

this can drop the use of cut (i.e. save a pipe):
                       BOOTMAC=$(echo $i | sed 's/^BOOTIF=..-//;s/-/:/g')

[actually, you could remove sed, too, and do it all in the shell,
 assuming you don't mind using a bash'ism (but this is too opaque, imho):
                       i=${i/#BOOTIF=??-/}
                       BOOTMAC=${i//-/:}


> +                    BOOTIF=$(grep -l $BOOTMAC /sys/class/net/eth*/address|rev|cut -d/ -f2|rev)
> +                    ;;
> +            esac
>         done
> +        configure_from_network $BOOTIF
>
>         # find all of the partitions on the system
>
> diff --git a/ovirt-host-creator/ovirt-pxe.sh b/ovirt-host-creator/ovirt-pxe.sh
> index 632ec5d..a586e87 100755
> --- a/ovirt-host-creator/ovirt-pxe.sh
> +++ b/ovirt-host-creator/ovirt-pxe.sh
> @@ -31,3 +31,11 @@ fi
> ISO=`create_iso $ISO` || exit 1
>
> livecd-iso-to-pxeboot $ISO
> +
> +# append BOOTIF with PXE MAC info
> +grep -q 'IPAPPEND 2' tftpboot/pxelinux.cfg/default
> +found=$?
> +if [ $found -ne 0 ]; then
> +    sed -i -e '/KERNEL/a \ \tIPAPPEND 2' tftpboot/pxelinux.cfg/default
> +fi

This should be equivalent, and hopefully more readable:
[dropped sed's -e, and removed the space-before-TAB]

    f=tftpboot/pxelinux.cfg/default
    grep -q 'IPAPPEND 2' $f || sed -i '/KERNEL/a \tIPAPPEND 2' $f