On Fri, Apr 05, 2024 at 08:10:02PM +0200, Stefano Brivio wrote:

On Fri, 22 Mar 2024 13:27:38 +1100 David Gibson wrote:

...

mbuto supports "auto" compression mode where we detect the fastest compressor and use it. This is structured a bit oddly - cpio_compress() first handles the case of an explicitly selected compressor, then handles the auto-detected case, redundantly actually implementing the compression once it has picked one.

Yes, we don't want to risk keeping around several formats if the archives are big... but you're not changing this, right?

I don't believe so, no.

...

Make this a bit clearer: first handle the "auto" case by calling out to the testing code, and using that to set the parameter for the specific compression path.

Signed-off-by: David Gibson --- mbuto | 53 ++++++++++++++++++++++++----------------------------- 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/mbuto b/mbuto index 49d032c..550f76e 100755 --- a/mbuto +++ b/mbuto @@ -566,31 +566,11 @@ cpio_init() { fi }

-# cpio_compress() - Compress archive, test available methods if none is selected +# compress_select() - Try compressors and pick the fastest

Now we can say we pick the fastest, but it's not clear in what: we care about decompression, so perhaps:

# compress_select() - Try compressors and pick the fastest to decompress image

?

I don't think that's accurate. AFAICT the code is selecting the fastest to compress, not decompress, and I didn't change that. I agree that optimizing decompression speed would make more sense. Honestly, I'm not really convinced that the auto mode is useful anyway: even if we changed to decompression speed, the speed on the location running mbuto isn't necessarily the same as on the target doing the decompression (though given mbuto's usage model, it's lkely). -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

On Thu, Apr 25, 2024 at 06:46:05AM +0200, Stefano Brivio wrote:

On Sat, 6 Apr 2024 14:06:39 +1100 David Gibson wrote:

...

On Fri, Apr 05, 2024 at 08:10:02PM +0200, Stefano Brivio wrote:

...

On Fri, 22 Mar 2024 13:27:38 +1100 David Gibson wrote:

...

mbuto supports "auto" compression mode where we detect the fastest compressor and use it. This is structured a bit oddly - cpio_compress() first handles the case of an explicitly selected compressor, then handles the auto-detected case, redundantly actually implementing the compression once it has picked one.

Yes, we don't want to risk keeping around several formats if the archives are big... but you're not changing this, right?

I don't believe so, no.

...

...

Make this a bit clearer: first handle the "auto" case by calling out to the testing code, and using that to set the parameter for the specific compression path.

Signed-off-by: David Gibson --- mbuto | 53 ++++++++++++++++++++++++----------------------------- 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/mbuto b/mbuto index 49d032c..550f76e 100755 --- a/mbuto +++ b/mbuto @@ -566,31 +566,11 @@ cpio_init() { fi }

-# cpio_compress() - Compress archive, test available methods if none is selected +# compress_select() - Try compressors and pick the fastest

Now we can say we pick the fastest, but it's not clear in what: we care about decompression, so perhaps:

# compress_select() - Try compressors and pick the fastest to decompress image

?

I don't think that's accurate. AFAICT the code is selecting the fastest to compress, not decompress,

No, why? Look:

__start=$("${CAT}" /proc/uptime) __start="${__start% *}"

for _ in $("${SEQ}" 1 5); do "${CP}" "${compress_test1}" "${compress_test2}" ${__cmd} --force -d -c "${compress_test2}" > /dev/null done

__end=$("${CAT}" /proc/uptime) __end="${__end% *}"

see the '-d' there ('-c' is usually '--stdout').

Oh, yes. My mistake.

...

and I didn't change that. I agree that optimizing decompression speed would make more sense.

Honestly, I'm not really convinced that the auto mode is useful anyway: even if we changed to decompression speed, the speed on the location running mbuto isn't necessarily the same as on the target doing the decompression (though given mbuto's usage model, it's lkely).

Well, it's exactly the intended usage -- any other usage would be rather tricky (at least, anything with a different architecture for the guest). That's how the current passt test suite uses it, too. And it really makes a difference if you're working with kselftests.

Hm, ok. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

On Fri, 22 Mar 2024 13:27:39 +1100 David Gibson wrote:

The cpio_init function is now only every called with the "discard" option. But, moreover, what it does is create an initial mostly empty archive which will just get overwritten by the final archive.

So, it's entirely unnecessary except for one subtlety. Our use of realpath when generating the final output requires that a file already exist in the output location. We can fix that by shuffling some things out of a subshell, removing the need for realpath.

Signed-off-by: David Gibson --- mbuto | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/mbuto b/mbuto index 550f76e..0c51e18 100755 --- a/mbuto +++ b/mbuto @@ -544,28 +544,6 @@ subopts_get() {

### CPIO #######################################################################

-# cpio_init() - Source existing CPIO archive, or create if needed -# $1: Path to CPIO archive, might exist, might be discarded if existing -cpio_init() { - if [ -f "${OUT}" ] && [ "${1}" != "discard" ]; then - info "Sourcing CPIO archive from ${OUT}" - - if ! "${GZIP}" -dfc "${OUT}" | - "${CPIO}" --quiet -iD "${wd}"; then - err "Invalid CPIO archive ${OUT}" - fi - else

This, and the second part of 1/4, remove a functionality which I accidentally broke in commit b87e4f2e6595 ("mbuto: Create working directory before profiles are sourced"): it was once possible to add contents to an existing initramfs archive. I guess I might be the only interactive user of mbuto at the moment, so I got slightly annoyed by the fact it didn't work anymore but I didn't really investigate further. I used it whenever an initramfs took more than 5-10 seconds to build, and I'd keep forgetting to add stuff. Note that the help message still describes this mode of operation: "Build initramfs image unless an existing one is passed." and in the "Examples": ./mbuto -f kata.img zsh_5.6.2-3_amd64.deb Install zsh package to pre-existing kata.img but I also realised it's not convenient to have this as default, because, especially for initramfs images that can be built faster, one might just want to rebuild a single binary and the image, repeatedly. Perhaps, instead of dropping this altogether, we could introduce a new -K ("keep") option that skips the unconditional wd assignment and calls cpio_init() (which becomes cpio_load() or something like that). -- Stefano

On Sat, 6 Apr 2024 14:11:09 +1100 David Gibson wrote:

On Fri, Apr 05, 2024 at 08:08:53PM +0200, Stefano Brivio wrote:

...

On Fri, 22 Mar 2024 13:27:39 +1100 David Gibson wrote:

...

The cpio_init function is now only every called with the "discard" option. But, moreover, what it does is create an initial mostly empty archive which will just get overwritten by the final archive.

So, it's entirely unnecessary except for one subtlety. Our use of realpath when generating the final output requires that a file already exist in the output location. We can fix that by shuffling some things out of a subshell, removing the need for realpath.

Signed-off-by: David Gibson --- mbuto | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/mbuto b/mbuto index 550f76e..0c51e18 100755 --- a/mbuto +++ b/mbuto @@ -544,28 +544,6 @@ subopts_get() {

### CPIO #######################################################################

-# cpio_init() - Source existing CPIO archive, or create if needed -# $1: Path to CPIO archive, might exist, might be discarded if existing -cpio_init() { - if [ -f "${OUT}" ] && [ "${1}" != "discard" ]; then - info "Sourcing CPIO archive from ${OUT}" - - if ! "${GZIP}" -dfc "${OUT}" | - "${CPIO}" --quiet -iD "${wd}"; then - err "Invalid CPIO archive ${OUT}" - fi - else

This, and the second part of 1/4, remove a functionality which I accidentally broke in commit b87e4f2e6595 ("mbuto: Create working directory before profiles are sourced"): it was once possible to add contents to an existing initramfs archive.

Yeah, I didn't track it to a specific point, but I assumed that it was built to add to an existing archive in the past. It looked fairly thoroughly broken to me.

Well, minus that change, that was the only way I used this tool for a while.

...

I guess I might be the only interactive user of mbuto at the moment, so I got slightly annoyed by the fact it didn't work anymore but I didn't really investigate further. I used it whenever an initramfs took more than 5-10 seconds to build, and I'd keep forgetting to add stuff.

So... in fact the kernel accepts not just a single cpio archive, but multiple. So rather than doing anything with archivemount or mangling the cpio, you could just append a secondary cpio with the additions.

Okay, good to know, but at least at a glance, I couldn't find out how to do this. I suppose we would need to decompress and compress the archive anyway, right?

...

Note that the help message still describes this mode of operation:

"Build initramfs image unless an existing one is passed."

and in the "Examples":

./mbuto -f kata.img zsh_5.6.2-3_amd64.deb Install zsh package to pre-existing kata.img

Ah, yes, they should be updated to match.

Right, with this updated I'm fine with this patch (and with the whole series with my comment to 3/4 addressed). I'm still convinced we should fix that and add it back, but it's not made more complicated by removing it now.

...

but I also realised it's not convenient to have this as default, because, especially for initramfs images that can be built faster, one might just want to rebuild a single binary and the image, repeatedly.

Perhaps, instead of dropping this altogether, we could introduce a new -K ("keep") option that skips the unconditional wd assignment and calls cpio_init() (which becomes cpio_load() or something like that).

-- Stefano