| Message ID | 18b10f667f2df986691ed299f7640724618ec95a.1696557834.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | automation: cleanup hardware based tests | expand |
On 06/10/2023 3:05 am, Marek Marczykowski-Górecki wrote: > diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh > index 5464d10fc343..842e6fae7204 100755 > --- a/automation/scripts/qubes-x86-64.sh > +++ b/automation/scripts/qubes-x86-64.sh > @@ -90,23 +90,18 @@ on_reboot = "destroy" > > domU_check=" > set -x -e > -ip link set eth0 up > -timeout 30s udhcpc -i eth0 > +interface=eth0 > +ip link set \"\$interface\" up > +timeout 30s udhcpc -i \"\$interface\" > pingip=\$(ip -o -4 r show default|cut -f 3 -d ' ') > ping -c 10 \"\$pingip\" > echo domU started > -cat /proc/interrupts > +pcidevice=\$(basename \$(readlink /sys/class/net/\$interface/device)) > +lspci -vs \$pcidevice Actually, I know I said I had no more comments, and maybe this is one for further cleanup rather than for now, but wouldn't we be much better using a heredoc? read -r -d '' domU_check <<"EOF" lorem ipsum, no \ escaping " or $ EOF If nothing else it would make the innards of the more readable as a script fragment, and less likely go to wrong with variable expansion in the wrong context. ~Andrew
On Thu, Oct 19, 2023 at 12:30:22AM +0100, Andrew Cooper wrote: > On 06/10/2023 3:05 am, Marek Marczykowski-Górecki wrote: > > diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh > > index 5464d10fc343..842e6fae7204 100755 > > --- a/automation/scripts/qubes-x86-64.sh > > +++ b/automation/scripts/qubes-x86-64.sh > > @@ -90,23 +90,18 @@ on_reboot = "destroy" > > > > domU_check=" > > set -x -e > > -ip link set eth0 up > > -timeout 30s udhcpc -i eth0 > > +interface=eth0 > > +ip link set \"\$interface\" up > > +timeout 30s udhcpc -i \"\$interface\" > > pingip=\$(ip -o -4 r show default|cut -f 3 -d ' ') > > ping -c 10 \"\$pingip\" > > echo domU started > > -cat /proc/interrupts > > +pcidevice=\$(basename \$(readlink /sys/class/net/\$interface/device)) > > +lspci -vs \$pcidevice > > Actually, I know I said I had no more comments, and maybe this is one > for further cleanup rather than for now, but wouldn't we be much better > using a heredoc? > > read -r -d '' domU_check <<"EOF" > > lorem ipsum, no \ escaping " or $ > > EOF > > If nothing else it would make the innards of the more readable as a > script fragment, and less likely go to wrong with variable expansion in > the wrong context. Some of those snippets have intentionally unescaped $ (as in - have it expanded in place), and IMO having different method depending whether you want to expand variables inside or not will be more error-prone. Syntax highlighting in an editor makes it rather easy to spot unescaped $ or such (and then decide whether it was intentional or not).
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml index 4b836bf04784..61e642cce0cc 100644 --- a/automation/gitlab-ci/test.yaml +++ b/automation/gitlab-ci/test.yaml @@ -195,8 +195,6 @@ adl-pci-pv-x86-64-gcc-debug: adl-pci-hvm-x86-64-gcc-debug: extends: .adl-x86-64 - variables: - PCIDEV_INTR: "MSI" script: - ./automation/scripts/qubes-x86-64.sh pci-hvm 2>&1 | tee ${LOGFILE} needs: diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh index 5464d10fc343..842e6fae7204 100755 --- a/automation/scripts/qubes-x86-64.sh +++ b/automation/scripts/qubes-x86-64.sh @@ -90,23 +90,18 @@ on_reboot = "destroy" domU_check=" set -x -e -ip link set eth0 up -timeout 30s udhcpc -i eth0 +interface=eth0 +ip link set \"\$interface\" up +timeout 30s udhcpc -i \"\$interface\" pingip=\$(ip -o -4 r show default|cut -f 3 -d ' ') ping -c 10 \"\$pingip\" echo domU started -cat /proc/interrupts +pcidevice=\$(basename \$(readlink /sys/class/net/\$interface/device)) +lspci -vs \$pcidevice " - if [ "$PCIDEV_INTR" = "MSI-X" ]; then + if [ -n "$PCIDEV_INTR" ]; then domU_check="$domU_check -grep -- '\\(-msi-x\\|PCI-MSI-X\\).*eth0' /proc/interrupts -" - elif [ "$PCIDEV_INTR" = "MSI" ]; then - # depending on the kernel version and domain type, the MSI can be - # marked as '-msi', 'PCI-MSI', or 'PCI-MSI-<SBDF>'; be careful to not match - # -msi-x nor PCI-MSI-X - domU_check="$domU_check -grep -- '\\(-msi \\|PCI-MSI\\( \\|-[^X]\\)\\).*eth0' /proc/interrupts +lspci -vs \$pcidevice | fgrep '$PCIDEV_INTR: Enable+' " fi domU_check="$domU_check diff --git a/automation/tests-artifacts/alpine/3.18.dockerfile b/automation/tests-artifacts/alpine/3.18.dockerfile index 333951d05e84..5f521572b8fb 100644 --- a/automation/tests-artifacts/alpine/3.18.dockerfile +++ b/automation/tests-artifacts/alpine/3.18.dockerfile @@ -33,6 +33,7 @@ RUN \ apk add pixman && \ apk add curl && \ apk add udev && \ + apk add pciutils && \ \ # Xen cd / && \
Checking /proc/interrupts is unreliable because different drivers set different names there. Install pciutils and use lspci instead. In fact, the /proc/interrupts content was confusing enough that adl-pci-hvm had it wrong (MSI-X is in use there). Fix this too. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- automation/gitlab-ci/test.yaml | 2 +-- automation/scripts/qubes-x86-64.sh | 19 +++++----------- automation/tests-artifacts/alpine/3.18.dockerfile | 1 +- 3 files changed, 8 insertions(+), 14 deletions(-)