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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, Jun 23, 2017 at 03:45:45PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v3 4/8] osstest: add a FreeBSD host install 
> recipe"):
> > +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.

I've added a pre-patch that introduces a sha256file helper in order to
calculate the hash of a file.

> > +    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.

I agree, I was surprised to find out I could do it like that.

> > +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
> ?

Yes, I've changed the path now to the suggested one.

> > +# 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)

Done:

find `dirname $sharedpath` -links 1 -ctime +7 -delete

> > +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 ?

I don't think it happens usually, but I guess the driver name could
change, and in turn make the device name change. As I said I think
this is mostly stable, so that users don't get nasty surprises.

> > +    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.

Yes, that's intended, it's the format of the install script. This
first part contains variables (ie: input data) used by the installer.
The second part is automatically executed chrooted into the root dir
of the newly installed system once done.

https://www.freebsd.org/cgi/man.cgi?query=bsdinstall

See the SCRIPTING section.

> > +# Setup serial console
> > +printf "%s" "-h -S$c{Baud}" >> /boot.config
> 
> Are you deliberately leaving /boot.config with no final newline ?

That's what I've usually done, but it doesn't matter.

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

It just consumes serial log space and has a nasty countdown.

Thanks, Roger.

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