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
