Re: [OE-core] [PATCH 1/1] wic: Content of the temporary updated fstab should be copied into the original not replacing it entirely.

["Mathieu Dubois-Briand" <mathieu.dubois-briand@bootlin.com>]

On Tue Sep 2, 2025 at 9:05 PM CEST, dani.barra25 via lists.openembedded.org wrote:
> From: Daniel Andrade <dani.barra25@gmail.com>
>
> Fixes [15947]
> The current functionality to update fstab generates a new temporary fstab with the new partition configuration.
> However, this file does not retain any metadata being it a completely new file.
> Because of this, when the rootfs plugin under `poky/scripts/lib/wic/plugins/source/rootfs.py` copies the file, it overrides the original fstab metadata.
> The patch removes implementation of msdos/ext code for fstab since it is not possible to append content without rewriting the file to preserve metadata.
>
> The timestamp applied to fstab is not the same as every other file.
> It seems like the `SOURCE_DATE_EPOCH` variable goes to the fallback timestamp SOURCE_DATE_EPOCH_FALLBACK.
> Since that variable is used everywhere, it is not the same value as REPRODUCIBLE_TIMESTAMP_ROOTFS under poky/meta/conf/bitbake.conf that is applied in every other file.
>
> Signed-off-by: Daniel Andrade <dani.barra25@gmail.com>
> ---

Hi Daniel,

I finally had a bit of time to have a deeper look at these changes and
at the wic.Wic.test_no_fstab_update test. It turns out the test is
correct and your patch is indeed introducing a regression.

So our first assumption was the test was wrong, as changing the
filesystem resulted in a test fail. But actually, if you change the
filesystem in the wic command file, you also have to change the command
extracting the file from the filesystem. E.g. for squashfs, you need to
replace "debugfs -R cat" with "sqfscat".

Now talking about your patch, my comments below.

> diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py
> index bf2c34d594..82d754835c 100644
> --- a/scripts/lib/wic/partition.py
> +++ b/scripts/lib/wic/partition.py
> @@ -131,7 +131,7 @@ class Partition():
>          partition command parameters.
>          """
>          self.updated_fstab_path = updated_fstab_path
> -        if self.updated_fstab_path and not (self.fstype.startswith("ext") or self.fstype == "msdos"):
> +        if self.updated_fstab_path:
>              self.update_fstab_in_rootfs = True
>  
>          if not self.source:
> @@ -295,15 +295,6 @@ class Partition():
>              (self.fstype, extraopts, rootfs, label_str, self.fsuuid, rootfs_dir)
>          exec_native_cmd(mkfs_cmd, native_sysroot, pseudo=pseudo)
>  
> -        if self.updated_fstab_path and self.has_fstab and not self.no_fstab_update:
> -            debugfs_script_path = os.path.join(cr_workdir, "debugfs_script")
> -            with open(debugfs_script_path, "w") as f:
> -                f.write("cd etc\n")
> -                f.write("rm fstab\n")
> -                f.write("write %s fstab\n" % (self.updated_fstab_path))
> -            debugfs_cmd = "debugfs -w -f %s %s" % (debugfs_script_path, rootfs)
> -            exec_native_cmd(debugfs_cmd, native_sysroot)
> -
>          mkfs_cmd = "fsck.%s -pvfD %s" % (self.fstype, rootfs)
>          exec_native_cmd(mkfs_cmd, native_sysroot, pseudo=pseudo)
>  
> @@ -400,10 +391,6 @@ class Partition():
>          mcopy_cmd = "mcopy -i %s -s %s/* ::/" % (rootfs, rootfs_dir)
>          exec_native_cmd(mcopy_cmd, native_sysroot)
>  
> -        if self.updated_fstab_path and self.has_fstab and not self.no_fstab_update:
> -            mcopy_cmd = "mcopy -m -i %s %s ::/etc/fstab" % (rootfs, self.updated_fstab_path)
> -            exec_native_cmd(mcopy_cmd, native_sysroot)
> -
>          chmod_cmd = "chmod 644 %s" % rootfs
>          exec_cmd(chmod_cmd)
>  

So you are removing an optimisation on ext and fat filesystems. This
might raise further questions from other reviewers, but if that's
needed, why not.

> diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py
> index e29f3a4c2f..3848af2a91 100644
> --- a/scripts/lib/wic/plugins/source/rootfs.py
> +++ b/scripts/lib/wic/plugins/source/rootfs.py
> @@ -223,8 +223,8 @@ class RootfsPlugin(SourcePlugin):
>              part.has_fstab = os.path.exists(os.path.join(new_rootfs, "etc/fstab"))
>              if part.update_fstab_in_rootfs and part.has_fstab and not part.no_fstab_update:
>                  fstab_path = os.path.join(new_rootfs, "etc/fstab")
> -                # Assume that fstab should always be owned by root with fixed permissions
> -                install_cmd = "install -m 0644 -p %s %s" % (part.updated_fstab_path, fstab_path)
> +                # We dont want any metadata of the updated fstab, just the one that already existed
> +                install_cmd = "cp --no-preserve=all %s %s" % (part.updated_fstab_path, fstab_path)

Now this is what makes the test fail. So I understand the switch from
install to cp, as it will reuse the same inode instead of creating a new
one. But this is also what causes the test regression.

When creating a filesystem image with a modified fstab, a copy of the
filesystem content is first made, so file modifications only affect this
specific image. But in order to gain time and disk space, this copy is
made using hardlinks: see use of copyhardlinktree() in
do_prepare_partition() from scripts/lib/wic/plugins/source/rootfs.py. So
as cp will keep the same inode, it means you will modify the content on
both the copy and the reference, and so all further generated image.

So I'm sorry, we will have to find another way to implement this change.

Thanks,
Mathieu

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com