Re: [OSSTEST PATCH] preseed_base: Use "keep" NIC NamePolicy when "force-mac-address"

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/06/2024 10:44 am, Anthony PERARD wrote:
> On Mon, Jun 17, 2024 at 04:34:09PM +0100, Andrew Cooper wrote:
>> On 17/06/2024 3:40 pm, Anthony PERARD wrote:
>>> diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
>>> index 3545f3fd..d974fea5 100644
>>> --- a/Osstest/Debian.pm
>>> +++ b/Osstest/Debian.pm
>>> @@ -972,7 +972,19 @@ END
>>>          # is going to be added to dom0's initrd, which is used by some 
>>> guests
>>>          # (created with ts-debian-install).
>>>          preseed_hook_installscript($ho, $sfx,
>>> -            '/usr/lib/base-installer.d/', '05ifnamepolicy', <<'END');
>>> +            '/usr/lib/base-installer.d/', '05ifnamepolicy',
>>> +            $ho->{Flags}{'force-mac-address'} ? <<'END' : <<'END');
>> The conditional looks suspicious if both options are <<'END'.
> That works fine, this pattern is already used in few places in osstest,
> like here:
> https://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=ts-host-install;h=0b6aaeeae228551064618abfa624321992a2eb2d;hb=HEAD#l240
>     >  $ho->{Flags}{'force-mac-address'} ? <<END : <<END);
>
> Or even here:
> https://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=ts-xen-build;h=c294a51eafc26e53b5417529b943224902870acf;hb=HEAD#l173
>     > buildcmd_stamped_logged(600, 'xen', 'configure', <<END,<<END,<<END);
>
>> Doesn't this just write 70-eth-keep-policy.link unconditionally?
> I've check that, on a different host, and the "mac" name policy is used
> as expected, so the file "70-eth-keep-policy.link" isn't created on that
> host.

This is horrifying.  Given a construct which specifically lets you
choose a semantically meaningful name, using END for all options is rude.

Despite the pre-existing antipatterns, it would be better to turn this
one into:

$ho->{Flags}{'force-mac-address'} ? <<'END_KEEP' : <<'END_MAC');

which gives the reader some chance of spotting that there are two
adjacent scripts and we're choosing one of them.

~Andrew