[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Ovirt-devel] [PATCH node] The node now passes in the mac address and iface names during identify.



"Darryl L. Pierce" <dpierce redhat com> wrote:
> From: Darryl Pierce <mcpierce threshold (none)>
> This works with the patch for the server the generates a configuration file
> for the node based on the interface names passed in with each mac address.
...

Hi Darryl,

Here are a couple of defensive fixes
and two questions:

> diff --git a/ovirt-identify-node/gather.c b/ovirt-identify-node/gather.c
> index 633d16e..9722e19 100644
> --- a/ovirt-identify-node/gather.c
> +++ b/ovirt-identify-node/gather.c
> @@ -210,6 +210,7 @@ get_nic_data(char *nic, nic_info_ptr nic_info_p)
>          libhal_device_get_property_string(hal_ctx, nic, "net.interface",
>                                            &dbus_error);
>      snprintf(nic_info_p->iface_name, BUFFER_LENGTH, "%s", interface);
> +

It'd be good not to add the blank line,
since that's the only change to this file.

>      bzero(&ifr, sizeof(struct ifreq));
>
>      sockfd = socket(AF_INET, SOCK_DGRAM, 0);
> diff --git a/scripts/ovirt-early b/scripts/ovirt-early
> index 8024b3b..0379946 100755
> --- a/scripts/ovirt-early
> +++ b/scripts/ovirt-early
> @@ -13,6 +13,12 @@
>  # size of the oVirt partition in megabytes
>  OVIRT_SIZE=64
>
> +get_mac_addresses() {
> +    macs=[`/sbin/ifconfig | awk '/HWaddr/ { print $5"="$1 }'`]
> +    macs=`echo $macs | sed 's/ /%2C/g'`
> +    macs=`echo $macs | sed 's/\:/%3A/g'`

This is fragile, since if there exist matching single-byte file names
like %, C, or hexadecimal numbers, then $macs will end up looking
not like you want, but just "0 A", since the square brackets make
the shell do filename globbing.

This should be equivalent but not susceptible, and forks
two fewer subshells (and no need to backslash-escape ":"):

  macs=$(ifconfig | awk '/HWaddr/ { print $5"="$1 }' \
         | tr '\n' ' ' | sed 's/ /%2C/g;s/:/%3A/g')
  macs="[$macs]"

Also, it's ok to omit /sbin/, since ovirt-early sources
/etc/init.d/functions, which sets PATH.

> +}
> +
>  configure_from_network() {
>      DEVICE=$1
>      if [ -n "$DEVICE" ]; then
> @@ -31,12 +37,19 @@ configure_from_network() {
>                  if [ -n "$SRV_HOST" -a -n "$SRV_PORT" ]; then
>                      printf .
>                      cfgdb=$(mktemp)
> +                    get_mac_addresses
>                      wget -q -O $cfgdb \
> -                      "http://$SRV_HOST:$SRV_PORT/ovirt/cfgdb/$(hostname)"
> +                      "http://$SRV_HOST:$SRV_PORT/ovirt/managed_node/config?host=$(hostname)&$macs"
>                      if [ $? -eq 0 ]; then
>                          printf .
> -                        echo "save" >> $cfgdb
> -                        augtool < $cfgdb > /dev/null 2>&1
> +                        bash $cfgdb

Not officially required, but good to be defensive and add double quotes,
in case TMPDIR (from which mktemp derives its name) ever contains
something bogus:

                          bash "$cfgdb"

> +                        if [ -f /var/tmp/pre-config-script ]; then
> +                            printf "loading kernel modules"
> +                            bash /var/tmp/pre-config-script > /dev/null 2>&1
> +                        fi

What output and diagnostics does the script print
that should always be ignored?

> +                        if [ -f /var/tmp/node-augtool ]; then
> +                            augtool < /var/tmp/node-augtool > /dev/null 2>&1

Same question here, for augtool.

> +                        fi
>                          if [ $? -eq 0 ]; then
>                              printf "remote config applied."
>                              return


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]