Re: [PATCH 4/4] Remove unnecessary cpio_init function

On Sat, 6 Apr 2024 14:11:09 +1100
David Gibson <david(a)gibson.dropbear.id.au> 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 <david(a)gibson.dropbear.id.au> 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 <david(a)gibson.dropbear.id.au>
 ---
  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