Re: [Xen-devel] [OSSTEST PATCH V3] ts-debian-hvm-install: use text installer frontend

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("[OSSTEST PATCH V3] ts-debian-hvm-install: use text installer 
frontend"):
> Factor out di_installcmdline_base in Debian.pm and use that in
> ts-debian-hvm-install. This should improve readability of d-i log in
> various Debian HVM testcases.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
...
> +sub di_installcmdline_base($;@) {
> +    my ($tho, %xopts) = @_;
> +    my @cl = qw(
> +                auto=true
> +                hw-detect/load_firmware=false
> +                DEBCONF_DEBUG=5
> +                );
> +    my $difront = get_host_property($tho,'DIFrontend','text');
> +    push @cl, ("DEBIAN_FRONTEND=$difront");
> +
> +    die "Both PreseedURL and PreseedFile are defined."
> +     if defined($xopts{PreseedURL}) && defined($xopts{PreseedFile});
> +
> +    push @cl, ("url=$xopts{PreseedURL}") if $xopts{PreseedURL};
> +
> +    push @cl, ("file=$xopts{PreseedFile}") if $xopts{PreseedFile};

Frankly, I disagree with Ian and I preferred the previous arrangement
where the caller would simply specify url= or file= as the case might
be, and di_installcmdline_base would specify "auto".

This way seems like a lot of pointless work - and it's less clear,
too, because to figure out what it's doing you have to chase some more
xopts settings.

But having said that I don't object to this version either.

> diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> index fec24f7..17adeb5 100755
> --- a/ts-debian-hvm-install
> +++ b/ts-debian-hvm-install
> @@ -86,18 +86,23 @@ END
>  
>  sub grub_cfg () {
>  
> +    my $cmdline = join ' ' , di_installcmdline_base($gho, PreseedFile => 
> "/preseed.cfg");
> +

There seems little point having di_installcmdline_base return an
array.  The array is just for convenience of construction.  If the
caller wants such an array too then putting the whole of a
space-joined list from di_installcmdline_base would be fine, since
there is no way to quote arguments containing spaces.

That would also mean that I wouldn't have to grumble about this line
being too long :-).

Thanks,
Ian.

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