Re: [PATCH v3] Set {ident}_suite runvar when install a Debian guest.

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-11-24 at 11:17 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH v3] Set {ident}_suite runvar when install a
> Debian guest."):
> > Currently those places which want this open code a lookup of the
> > {ident}_suite runvar with a fallback to the configuration file.
> > 
> > However selecthost was missing such a lookup in the case where it is
> > constructing a nested L1 host (which begins from the selectguest
> > template), which lead to ts-xen-install on Jessie missing the
> > installation of libnl-route-3-200.
> > 
> > Fix this by providing debian_guest_suite($gho) which as well as
> > initialising $gho->{Suite} stores an {ident}_suite runvar (taking care
> > to handle the case where one is already set by e.g. make-flight). For
> > convenience debian_guest_suite() also returns the suite name.
> > 
> > ts-debian-install, ts-debian-di-install and ts-debian-hvm-install now
> > use debian_guest_suite instead of open coding the lookup.
> > 
> > The final piece of the puzzle is to have selectguest() pickup the
> > {ident}_suite runvar (if it is set) and initialise $gho->{Suite} from
> > it.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > I've kicked off an adhoc run testing:
> > +    test-amd64-amd64-qemuu-nested);; # nested
> > +    test-amd64-amd64-xl-qemuu-debianhvm-amd64);; # regular HVM
> > +    test-amd64-amd64-xl-qcow2);; # DI install
> > +    test-amd64-amd64-xl);; # Normal PV

FYI this last one failed, I'll investigate and no doubt there will be a v4.
I'll fix the style stuff as I go.

> > with this change.
> 
> This looks like you wrote a case statement in sh.  Did you know about
> OSSTEST_JOBS_ONLY (see cs-job-create) ?

Indeed, I hacked it into make-flights filter hooks. I'll investigate
OSSTEST_JOBS_ONLY for next time.

> FFR, I have some style tips:
> 
> > +sub debian_guest_suite ($) {
> > +    my ($gho) = @_;
> > +
> > +    return $gho->{Suite} if $gho->{Suite};
> > +
> > +    $gho->{Suite} = guest_var($gho,'suite',undef);
> 
> If you do this instead
> 
>   +    $gho->{Suite} //= guest_var($gho,'suite',undef);
> 
> the previous line (`return ... if') is not needed.

Done. (I suppose this will elide the call to guest_var if ->{Suite} is
already defined, like an || would).

> > +    if ( ! $gho->{Suite} ) {
> 
> This is stylistically odd IMO.  Seems to have been written by a
> hypervisor maintainer :-).

I think you are referring to the too many spaces and would normally do
	if (!$gho->{Suite}) {
or was there something else?

Ian.


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