Re: [Xen-devel] [PATCH v3 4/8] osstest: add a FreeBSD host install recipe

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v3 4/8] osstest: add a FreeBSD host install 
recipe"):
> The installation is performed using the bsdinstall tool, which is part
> of the FreeBSD base system. The installer image is setup with the
> osstest ssh keys and sshd enabled by default, which allows the test
> harness to just ssh into the box, create the install config file and
> launch the scripted install.

thanks.

> +# The installer image must be named 'install.img', and the sets
> +# 'kernel.txz', 'base.txz' and finally the 'MANIFEST' file that contains
> +# the checksums.

Thanks for the good doc comment.

> +sub setup_netboot_installer () {
> +    my $image = "$path_prefix/install.img";
> +    my $pxeimg = target_tftp_prefix($ho) . "--freebsd.img";
> +    my $hash = `sha256sum $image | head -c 16` or die $!;

You should do this `  -' stripping in Perl.  That way, also, you won't
lose the exit status from sha256sum as you do here.

> +    my $tftp_freebsd = 
> "$ho->{Tftp}{Path}/$ho->{Tftp}{TmpDir}/freebsd-images/";
> +    my $script = <<'END';
> +basedir=$0
> +imagepath=$1
> +sharedpath=$2
> +targetpath=$3

Please pass a dummy argument to the script (I usually use `x')
and shift all these arguments along.  Passing a real argument as $0 is
IMO strange.

> +cd $basedir
> +if [ ! -f $sharedpath ]; then
> +    mkdir -p `dirname $sharedpath`
> +    cp $imagepath $sharedpath

Please use the copy-to-tempfile-and-rename pattern.

AFAICT your filename pattern for the per-hash filename is
   $tftp_freebsd/$r{arch}/$hash/install.img

Is there some reason why this needs
 - to be qualified with $r{arch}
 - to have a bunch of per-hash directories containing one file each
?

Why not
   $tftp_freebsd/by-hash/$hash.img
?

> +# Dir format from basedir is $arch/$hash/install.img
> +for hashdir in `find -mindepth 2 -maxdepth 2 -type d`; do

1. use xargs or -exec -rm
2. use find -links
3. add -ctime +7 or something, so we don't delete things which
   have just been added (or used)

> +    my @cmd = ( "with-lock-ex", "-w", "$tftp_freebsd/lock",
> +                "bash", "-exc", "$script",
> +                "$tftp_freebsd", "$image", "$r{arch}/$hash/install.img",
> +                "$ho->{Tftp}{Path}/$pxeimg" );

Use of qw() would make this a lot less visually noisy.

> +    ensuredir("$tftp_freebsd");

" " are redundant in Perl.  It's not sh :-).

> +    logm("Trying to find a disk to install to");
> +    $disk = target_cmd_output_root($ho, <<END, 30);
> +for disk in @disk_names; do

Did you want to add set -e ?

> +    if [ -c "/dev/\$disk" ]; then
> +        echo \$disk
> +        exit 0
> +    fi
> +done
> +exit 1
> +END
> +    defined($disk) or die "Unable to find a valid disk";

target_cmd_output_root will never return undef.  I think this
error check is redundant, therefore.

> +    logm("Trying to figure out primary nic device name");
> +    $nic = target_cmd_output_root($ho, <<END, 30);
> +nics=`ifconfig -l`

I think this definitely needs set -e.

> +for nic in \$nics; do
> +    addr=`ifconfig \$nic inet|grep inet|awk {'print \$2'}`
> +    if [ "\$addr" = "$ho->{Ip}" ]; then
> +        echo \$nic
> +        exit 0
> +    fi
> +done
> +exit 1
> +END

Is it likely that the disk or network device name, for a particular
device, will change, for example across versions of FreeBSD ?

> +    logm("Uploading the install sets to the system");
> +    target_cmd_root($ho, <<END, 30);
> +mkdir -p $target_sets

Missing set -e.

> +    logm("Creating the installer script");
> +    target_putfilecontents_root_stash($ho, 10, <<END, '~/installscript');
> +set -a
> +BSDINSTALL_DISTDIR="$target_sets"
> +ZFSBOOT_DISKS="$disk"
> +DISTRIBUTIONS="@sets"
> +nonInteractive=1
> +
> +#!/bin/sh
> +set -ex

There's a #! halfway through this script.

> +# Setup serial console
> +printf "%s" "-h -S$c{Baud}" >> /boot.config

Are you deliberately leaving /boot.config with no final newline ?

> +cat << ENDBOOT >> /boot/loader.conf
> +boot_serial="YES"
> +comconsole_speed="$c{Baud}"
> +console="comconsole"
> +boot_verbose="YES"
> +beastie_disable="YES"

:-) re beastie.


Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel