| Message ID | 20240617144051.29547-1-anthony@xenproject.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [OSSTEST] preseed_base: Use "keep" NIC NamePolicy when "force-mac-address" | expand |
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'. Doesn't this just write 70-eth-keep-policy.link unconditionally? ~Andrew > +#!/bin/sh -e > +linkfile=/target/etc/systemd/network/70-eth-keep-policy.link > +mkdir -p `dirname $linkfile` > +cat > $linkfile <<EOF > +[Match] > +Type=ether > +Driver=!vif > +[Link] > +NamePolicy=keep > +EOF > +END > #!/bin/sh -e > linkfile=/target/etc/systemd/network/90-eth-mac-policy.link > mkdir -p `dirname $linkfile` >
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. Cheers,
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
On Tue, Jun 18, 2024 at 12:04:05PM +0100, Andrew Cooper wrote: > 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'); I've push the patch with this change. Cheers,
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'); +#!/bin/sh -e +linkfile=/target/etc/systemd/network/70-eth-keep-policy.link +mkdir -p `dirname $linkfile` +cat > $linkfile <<EOF +[Match] +Type=ether +Driver=!vif +[Link] +NamePolicy=keep +EOF +END #!/bin/sh -e linkfile=/target/etc/systemd/network/90-eth-mac-policy.link mkdir -p `dirname $linkfile` diff --git a/ts-host-install b/ts-host-install index 0b6aaeea..fbbfeecc 100755 --- a/ts-host-install +++ b/ts-host-install @@ -248,7 +248,21 @@ END print CANARY "\n# - canary - came via initramfs\n" or die $!; close CANARY or die $!; - if ($ho->{Suite} !~ m/lenny|squeeze|wheezy|jessie|stretch|buster/) { + if ($ho->{Flags}{'force-mac-address'}) { + # When we have to set a MAC address, make sure that the interface keep + # the original name that the kernel give, "eth0". There should only be + # one interface in the ysstem in this case, so no risk of mixup. + system_checked(qw(mkdir -p --), "$initrd_overlay.d/lib/systemd/network"); + file_simple_write_contents + ("$initrd_overlay.d/lib/systemd/network/70-eth-keep-policy.link", + <<END); +[Match] +Type=ether +Driver=!vif +[Link] +NamePolicy=keep +END + } elsif ($ho->{Suite} !~ m/lenny|squeeze|wheezy|jessie|stretch|buster/) { # Switch to more predictale nic name based on mac address, instead of the # policy "onboard" which can try to set the same name ("eno1") to two # differents nic, or "slot". New names are "enx$mac".