For Qubes, this requires switching from sh to bash.

This reduces the number of times the target filename needs to be written to 1.

Expand the comment to explain the concatination constraints.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

I would like to find a slightly nicer way of conditional parts, but nothing
comes to mind.
---
 automation/scripts/qubes-x86-64.sh             | 14 +++++++++-----
 automation/scripts/xilinx-smoke-dom0-x86_64.sh | 16 +++++++++-------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
index 10af274a0ba7..1dd3f48b3d29 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 
 set -ex -o pipefail
 
@@ -187,10 +187,14 @@ Kernel \r on an \m (\l)
     rm -rf rootfs
 fi
 
-# Dom0 rootfs
-cp binaries/ucode.cpio binaries/dom0-rootfs.cpio.gz
-cat binaries/rootfs.cpio.gz >> binaries/dom0-rootfs.cpio.gz
-cat binaries/xen-tools.cpio.gz >> binaries/dom0-rootfs.cpio.gz
+# Dom0 rootfs.  The order or concatination is important; ucode wants to come
+# first, and all uncompressed must be ahead of compressed.
+parts=(
+    binaries/ucode.cpio
+    binaries/rootfs.cpio.gz
+    binaries/xen-tools.cpio.gz
+)
+cat "${parts[@]}" > binaries/dom0-rootfs.cpio.gz
 
 # test-local configuration
 mkdir -p rootfs
diff --git a/automation/scripts/xilinx-smoke-dom0-x86_64.sh b/automation/scripts/xilinx-smoke-dom0-x86_64.sh
index 8f02fa73bd06..0fbabb41054a 100755
--- a/automation/scripts/xilinx-smoke-dom0-x86_64.sh
+++ b/automation/scripts/xilinx-smoke-dom0-x86_64.sh
@@ -103,13 +103,15 @@ find . | cpio -H newc -o | gzip >> ../binaries/domU-rootfs.cpio.gz
 cd ..
 rm -rf rootfs
 
-# Dom0 rootfs
-cp binaries/ucode.cpio binaries/dom0-rootfs.cpio.gz
-cat binaries/rootfs.cpio.gz >> binaries/dom0-rootfs.cpio.gz
-cat binaries/xen-tools.cpio.gz >> binaries/dom0-rootfs.cpio.gz
-if [[ "${TEST}" == argo ]]; then
-    cat binaries/argo.cpio.gz >> binaries/dom0-rootfs.cpio.gz
-fi
+# Dom0 rootfs.  The order or concatination is important; ucode wants to come
+# first, and all uncompressed must be ahead of compressed.
+parts=(
+    binaries/ucode.cpio
+    binaries/rootfs.cpio.gz
+    binaries/xen-tools.cpio.gz
+)
+[[ "${TEST}" == argo ]] && parts+=(binaries/argo.cpio.gz)
+cat "${parts[@]}" > binaries/dom0-rootfs.cpio.gz
 
 # test-local configuration
 mkdir -p rootfs
-- 
2.39.5

On Thu, May 22, 2025 at 06:36:39PM +0100, Andrew Cooper wrote:
> For Qubes, this requires switching from sh to bash.
> 
> This reduces the number of times the target filename needs to be written to 1.
> 
> Expand the comment to explain the concatination constraints.

Isn't the correct spelling "concatenation"? Same for the two comments.

> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> I would like to find a slightly nicer way of conditional parts, but nothing
> comes to mind.

Well, one way I can think of is having a new variable which can carry
the rootfs part associated with a particular test, then that variable
can be updated at the time we configure for that test. Something like:

# init
declare -a append_rootfs_part
# or append_rootfs_part=() is probably fine too.

case $test in
  argo)
    append_rootfs_part+=(argo.cpio.gz)
    # ... other test configuration
    ;;
esac

# Dom0 rootfs
parts=(
    rootfs.cpio.gz
    xen-tools.cpio.gz
    "${append_rootfs_part[@]}"
)

And that should works fine, even if there isn't any extra rootfs part.

> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> index 10af274a0ba7..1dd3f48b3d29 100755
> --- a/automation/scripts/qubes-x86-64.sh
> +++ b/automation/scripts/qubes-x86-64.sh
> @@ -187,10 +187,14 @@ Kernel \r on an \m (\l)
>      rm -rf rootfs
>  fi
>  
> -# Dom0 rootfs
> -cp binaries/ucode.cpio binaries/dom0-rootfs.cpio.gz
> -cat binaries/rootfs.cpio.gz >> binaries/dom0-rootfs.cpio.gz
> -cat binaries/xen-tools.cpio.gz >> binaries/dom0-rootfs.cpio.gz
> +# Dom0 rootfs.  The order or concatination is important; ucode wants to come

                             ^ of concatenation

Same typo in the other comment.

Beside the typo, patch looks fine:
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD

On Tue, 27 May 2025, Anthony PERARD wrote:
> On Thu, May 22, 2025 at 06:36:39PM +0100, Andrew Cooper wrote:
> > For Qubes, this requires switching from sh to bash.
> > 
> > This reduces the number of times the target filename needs to be written to 1.
> > 
> > Expand the comment to explain the concatination constraints.
> 
> Isn't the correct spelling "concatenation"? Same for the two comments.
> 
> > 
> > No functional change.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > I would like to find a slightly nicer way of conditional parts, but nothing
> > comes to mind.
> 
> Well, one way I can think of is having a new variable which can carry
> the rootfs part associated with a particular test, then that variable
> can be updated at the time we configure for that test. Something like:
> 
> # init
> declare -a append_rootfs_part
> # or append_rootfs_part=() is probably fine too.
> 
> case $test in
>   argo)
>     append_rootfs_part+=(argo.cpio.gz)
>     # ... other test configuration
>     ;;
> esac
> 
> # Dom0 rootfs
> parts=(
>     rootfs.cpio.gz
>     xen-tools.cpio.gz
>     "${append_rootfs_part[@]}"
> )
> 
> And that should works fine, even if there isn't any extra rootfs part.
> 
> > diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> > index 10af274a0ba7..1dd3f48b3d29 100755
> > --- a/automation/scripts/qubes-x86-64.sh
> > +++ b/automation/scripts/qubes-x86-64.sh
> > @@ -187,10 +187,14 @@ Kernel \r on an \m (\l)
> >      rm -rf rootfs
> >  fi
> >  
> > -# Dom0 rootfs
> > -cp binaries/ucode.cpio binaries/dom0-rootfs.cpio.gz
> > -cat binaries/rootfs.cpio.gz >> binaries/dom0-rootfs.cpio.gz
> > -cat binaries/xen-tools.cpio.gz >> binaries/dom0-rootfs.cpio.gz
> > +# Dom0 rootfs.  The order or concatination is important; ucode wants to come
> 
>                              ^ of concatenation
> 
> Same typo in the other comment.
> 
> Beside the typo, patch looks fine:
> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

On Tue, May 27, 2025 at 04:01:34PM +0200, Anthony PERARD wrote:
> On Thu, May 22, 2025 at 06:36:39PM +0100, Andrew Cooper wrote:
> > For Qubes, this requires switching from sh to bash.
> > 
> > This reduces the number of times the target filename needs to be written to 1.
> > 
> > Expand the comment to explain the concatination constraints.
> 
> Isn't the correct spelling "concatenation"? Same for the two comments.
> 
> > 
> > No functional change.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > I would like to find a slightly nicer way of conditional parts, but nothing
> > comes to mind.
> 
> Well, one way I can think of is having a new variable which can carry
> the rootfs part associated with a particular test, then that variable
> can be updated at the time we configure for that test. Something like:
> 
> # init
> declare -a append_rootfs_part
> # or append_rootfs_part=() is probably fine too.
> 
> case $test in
>   argo)
>     append_rootfs_part+=(argo.cpio.gz)
>     # ... other test configuration
>     ;;
> esac
> 
> # Dom0 rootfs
> parts=(
>     rootfs.cpio.gz
>     xen-tools.cpio.gz
>     "${append_rootfs_part[@]}"
> )
> 
> And that should works fine, even if there isn't any extra rootfs part.

That would work for compressed parts, but not for uncompressed - which
need to come before all compressed. But maybe there could be two arrays
- one for uncompressed and another for compressed? Then, each could be
extended anywhere, without messing the order.

> 
> > diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> > index 10af274a0ba7..1dd3f48b3d29 100755
> > --- a/automation/scripts/qubes-x86-64.sh
> > +++ b/automation/scripts/qubes-x86-64.sh
> > @@ -187,10 +187,14 @@ Kernel \r on an \m (\l)
> >      rm -rf rootfs
> >  fi
> >  
> > -# Dom0 rootfs
> > -cp binaries/ucode.cpio binaries/dom0-rootfs.cpio.gz
> > -cat binaries/rootfs.cpio.gz >> binaries/dom0-rootfs.cpio.gz
> > -cat binaries/xen-tools.cpio.gz >> binaries/dom0-rootfs.cpio.gz
> > +# Dom0 rootfs.  The order or concatination is important; ucode wants to come
> 
>                              ^ of concatenation
> 
> Same typo in the other comment.
> 
> Beside the typo, patch looks fine:
> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
> 
> Thanks,
> 
> -- 
> Anthony PERARD

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

On 27/05/2025 4:19 pm, Marek Marczykowski-Górecki wrote:
> On Tue, May 27, 2025 at 04:01:34PM +0200, Anthony PERARD wrote:
>> On Thu, May 22, 2025 at 06:36:39PM +0100, Andrew Cooper wrote:
>>> For Qubes, this requires switching from sh to bash.
>>>
>>> This reduces the number of times the target filename needs to be written to 1.
>>>
>>> Expand the comment to explain the concatination constraints.
>> Isn't the correct spelling "concatenation"? Same for the two comments.
>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> I would like to find a slightly nicer way of conditional parts, but nothing
>>> comes to mind.
>> Well, one way I can think of is having a new variable which can carry
>> the rootfs part associated with a particular test, then that variable
>> can be updated at the time we configure for that test. Something like:
>>
>> # init
>> declare -a append_rootfs_part
>> # or append_rootfs_part=() is probably fine too.
>>
>> case $test in
>>   argo)
>>     append_rootfs_part+=(argo.cpio.gz)
>>     # ... other test configuration
>>     ;;
>> esac
>>
>> # Dom0 rootfs
>> parts=(
>>     rootfs.cpio.gz
>>     xen-tools.cpio.gz
>>     "${append_rootfs_part[@]}"
>> )
>>
>> And that should works fine, even if there isn't any extra rootfs part.
> That would work for compressed parts, but not for uncompressed - which
> need to come before all compressed. But maybe there could be two arrays
> - one for uncompressed and another for compressed? Then, each could be
> extended anywhere, without messing the order.

Hmm, two might work, but they surely need to not be quoted when forming
parts=(), or having multiple entries will go wrong on the eventual cat
command line.

~Andrew

On Tue, May 27, 2025 at 04:24:57PM +0100, Andrew Cooper wrote:
> On 27/05/2025 4:19 pm, Marek Marczykowski-Górecki wrote:
> > On Tue, May 27, 2025 at 04:01:34PM +0200, Anthony PERARD wrote:
> >> On Thu, May 22, 2025 at 06:36:39PM +0100, Andrew Cooper wrote:
> >>> For Qubes, this requires switching from sh to bash.
> >>>
> >>> This reduces the number of times the target filename needs to be written to 1.
> >>>
> >>> Expand the comment to explain the concatination constraints.
> >> Isn't the correct spelling "concatenation"? Same for the two comments.
> >>
> >>> No functional change.
> >>>
> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>> ---
> >>> I would like to find a slightly nicer way of conditional parts, but nothing
> >>> comes to mind.
> >> Well, one way I can think of is having a new variable which can carry
> >> the rootfs part associated with a particular test, then that variable
> >> can be updated at the time we configure for that test. Something like:
> >>
> >> # init
> >> declare -a append_rootfs_part
> >> # or append_rootfs_part=() is probably fine too.
> >>
> >> case $test in
> >>   argo)
> >>     append_rootfs_part+=(argo.cpio.gz)
> >>     # ... other test configuration
> >>     ;;
> >> esac
> >>
> >> # Dom0 rootfs
> >> parts=(
> >>     rootfs.cpio.gz
> >>     xen-tools.cpio.gz
> >>     "${append_rootfs_part[@]}"
> >> )
> >>
> >> And that should works fine, even if there isn't any extra rootfs part.
> > That would work for compressed parts, but not for uncompressed - which
> > need to come before all compressed. But maybe there could be two arrays
> > - one for uncompressed and another for compressed? Then, each could be
> > extended anywhere, without messing the order.

You could use "${append_rootfs_part:#*.gz}" and
"${(M)append_rootfs_part:#*.gz}" to grab the uncompressed part then the
compressed part... on zsh :-). But something similar could be codded in
bash. But I guess two variables will be more acceptable.

> Hmm, two might work, but they surely need to not be quoted when forming
> parts=(), or having multiple entries will go wrong on the eventual cat
> command line.

The double quote are needed! Well not really because it's very unlikely
that there's going to be blanked characters in paths to parts.

Maybe this will help understand how "${var[@]}" is expended into
multiple arguments:

# Testing with just for loop, also show the difference between
#  "${v[@]}" and "${v[*]}":

$ parts=(one two)
$ for i in "${parts[@]}"; do echo "- $i"; done
- one
- two
$ for i in "${parts[*]}"; do echo "- $i"; done
- one two
$ extra=("first extra" "second extra")
$ for i in "${extra[@]}"; do echo "- $i"; done
- first extra
- second extra
$ parts=(one "${extra[@]}" two)
$ for i in "${parts[@]}"; do echo "- $i"; done
- one
- first extra
- second extra
- two

# And now with function

$ print_array(){ for i in "$@"; do echo "- $i"; done; }
$ print_array "${parts[@]}"
- one
- first extra
- second extra
- two
$ print_array ${parts[@]}
- one
- first
- extra
- second
- extra
- two

Cheers,

-- 
Anthony PERARD

On 28/05/2025 10:45 am, Anthony PERARD wrote:
> On Tue, May 27, 2025 at 04:24:57PM +0100, Andrew Cooper wrote:
>> On 27/05/2025 4:19 pm, Marek Marczykowski-Górecki wrote:
>>> On Tue, May 27, 2025 at 04:01:34PM +0200, Anthony PERARD wrote:
>>>> On Thu, May 22, 2025 at 06:36:39PM +0100, Andrew Cooper wrote:
>>>>> For Qubes, this requires switching from sh to bash.
>>>>>
>>>>> This reduces the number of times the target filename needs to be written to 1.
>>>>>
>>>>> Expand the comment to explain the concatination constraints.
>>>> Isn't the correct spelling "concatenation"? Same for the two comments.
>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> ---
>>>>> I would like to find a slightly nicer way of conditional parts, but nothing
>>>>> comes to mind.
>>>> Well, one way I can think of is having a new variable which can carry
>>>> the rootfs part associated with a particular test, then that variable
>>>> can be updated at the time we configure for that test. Something like:
>>>>
>>>> # init
>>>> declare -a append_rootfs_part
>>>> # or append_rootfs_part=() is probably fine too.
>>>>
>>>> case $test in
>>>>   argo)
>>>>     append_rootfs_part+=(argo.cpio.gz)
>>>>     # ... other test configuration
>>>>     ;;
>>>> esac
>>>>
>>>> # Dom0 rootfs
>>>> parts=(
>>>>     rootfs.cpio.gz
>>>>     xen-tools.cpio.gz
>>>>     "${append_rootfs_part[@]}"
>>>> )
>>>>
>>>> And that should works fine, even if there isn't any extra rootfs part.
>>> That would work for compressed parts, but not for uncompressed - which
>>> need to come before all compressed. But maybe there could be two arrays
>>> - one for uncompressed and another for compressed? Then, each could be
>>> extended anywhere, without messing the order.
> You could use "${append_rootfs_part:#*.gz}" and
> "${(M)append_rootfs_part:#*.gz}" to grab the uncompressed part then the
> compressed part... on zsh :-). But something similar could be codded in
> bash. But I guess two variables will be more acceptable.

I believe there's a restriction that only one type of compression can be
used, but I don't particularly fancy tying it to gz specifically.

Something else to look at in some copious free time is .xz or so.  For
test-artefacts its surely a size win, but whether it's better overall
depends on whether using xz in this script doesn't undo the
optimisations we've been trying.  Once this series is in, we're down to
a handful of tiny text files, so I expect it to be in the noise.

>> Hmm, two might work, but they surely need to not be quoted when forming
>> parts=(), or having multiple entries will go wrong on the eventual cat
>> command line.
> The double quote are needed!

Yes, sorry.  That was a stupid suggestion of mine.  I really ought to
know how "$@" works by now...

~Andrew