mke2fs: the -d option can now handle tarball input

[josch]

This is an independent implementation that does the same thing as #107. Unfortunately I wasn't aware that e2fsprogs had a github repository (http://e2fsprogs.sourceforge.net/ only mentions the kernel.org repo). Differences of this approach compared to #107 are:

• no hard dependency on libarchive via some #ifdefs
• re-uses the -d option instead of adding a new option
• supports reading the archive from stdin using -
• comes with several test cases checking that
  • a roundtrip from tar to ext4 to tar produces bit-by-bit identical results
  • extended attributes work
  • gnu tar and pax work
  • character devices and symlinks work

Just as @russdill I'm looking for comments on this. Thanks!

[josch]

Use dlopen instead of linking against libarchive to keep runtime dependencies minimal as requested in #107 (comment)

[josch]

Hi, @tytso, could you give some feedback for this pull request or for #107 so that I can work on fixing remaining issues? Thanks!

[tytso]

Sorry for the delay in looking at this pull request. Things have been pretty busy. So some comments about this change.

First of all, when I tried doing a trial merge, I got test failures for the m_rootgnutar test. There are a lot of failures of the form:

dump_file: Operation not permitted while changing ownership of test.img.dir/progs/hold_inode.c
dump_file: Operation not permitted while changing ownership of test.img.dir/progs/random_exercise.c
....

Secondly, please test to make sure that (a) e2fsprogs builds with and without libarchive installed. One of the really nasty things about configure scripts is that very often, people are careless about them, and the autoconf scripts slow down the build process (since you have to run configure script, which takes time), but it doesn't buy you anything in terms of portability if the build crashes and burns if the build environment does not exactly match the developer's. If it's going to be non-portable, why waste time with the configure script? Secondly, after fixing the configure script, we then find that "make check" fails all of the three new tests, since m_rootgnutar, m_rootpaxtar, and m_roottar are written assuming that the libarchive functionality is present.

While I do appreciate adding the dlopen() support, one question that comes to mind is how stable is the libarchive ABI. Given that the the SOVERSION is up to 13, it causes me to wonder whether it's like openssl, where the ABI is so badly designed that a SOBUMP is needed at essentially every single release. Also, the man page needs to warn that a particular bit of functionality might not be present --- either because at compile-time, the archive.h header file isn't there, in which case the functionality will never be present, or at run-time, because the shared library for libarchive isn't -present, in which case presumably we should get a nice warning message a file was passed to the -d option, but we can't handle it because libarchive isn't available --- and not a confusing error message such as:

__populate_fs: Not a directory while changing working directory to "test.img.tar"

[josch]

Sorry for the delay in looking at this pull request. Things have been pretty busy.

No problem, same over here. :)

So some comments about this change.

Thank you for your review!

First of all, when I tried doing a trial merge, I got test failures for the m_rootgnutar test. There are a lot of failures of the form:

dump_file: Operation not permitted while changing ownership of test.img.dir/progs/hold_inode.c
dump_file: Operation not permitted while changing ownership of test.img.dir/progs/random_exercise.c
....

This should be fixed now.

Secondly, please test to make sure that (a) e2fsprogs builds with and without libarchive installed. One of the really nasty things about configure scripts is that very often, people are careless about them, and the autoconf scripts slow down the build process (since you have to run configure script, which takes time), but it doesn't buy you anything in terms of portability if the build crashes and burns if the build environment does not exactly match the developer's. If it's going to be non-portable, why waste time with the configure script?

I verified that e2fsprog builds with and without libarchive installed.

Secondly, after fixing the configure script, we then find that "make check" fails all of the three new tests, since m_rootgnutar, m_rootpaxtar, and m_roottar are written assuming that the libarchive functionality is present.

Fixed now as well.

While I do appreciate adding the dlopen() support, one question that comes to mind is how stable is the libarchive ABI. Given that the the SOVERSION is up to 13, it causes me to wonder whether it's like openssl, where the ABI is so badly designed that a SOBUMP is needed at essentially every single release.

The last SOVERSION bump happened 10 years ago. I asked about the ABI stability here but got no reply yet: libarchive/libarchive#1854

Also, the man page needs to warn that a particular bit of functionality might not be present --- either because at compile-time, the archive.h header file isn't there, in which case the functionality will never be present, or at run-time, because the shared library for libarchive isn't -present,

I added this information to the man page.

in which case presumably we should get a nice warning message a file was passed to the -d option, but we can't handle it because libarchive isn't available and not a confusing error message such as:

__populate_fs: Not a directory while changing working directory to "test.img.tar"

I was unable to trigger this error. Do you remember what you did to get it? In any case, I changed some of the code paths surrounding __populate_fs so this might not get generated anymore.

[josch]

I'm submitted my patch to the linux-ext4 list to get more feedback: https://lore.kernel.org/linux-ext4/20230620121641.469078-1-josch@mister-muffin.de/

[guillemj]

While I do appreciate adding the dlopen() support, one question that comes to mind is how stable is the libarchive ABI. Given that the the SOVERSION is up to 13, it causes me to wonder whether it's like openssl, where the ABI is so badly designed that a SOBUMP is needed at essentially every single release.

In general I think dlopen()ing a specific shared library via its full SONAME from an external project where there is no tight coordination going on is the wrong approach because it's crossing an ABI boundary with no checks in place, which requires manual patching when the SONAME gets bumped even if the used symbols have not seen any ABI or ABI change, and the code will not automatically pick up ABI changes that are API compatible. I think a better solution is to create a shared object "module" or "plugin" exposing the functions that you need that links against the libarchive library as usual, which will mean the external project can always be used safely, or the compiled will just barf, and this module within the project boundaries can be safely dlopen()ed if present. Then when packaging this module can be split into its own package and installed if desired f.ex, which will also make it possible to automatically pick up the required dependencies w/o having to hardcode those as well.

[evelikov]

The libarchive ABI has been fairly stable for nearly a decade now and I would be in favour of seeing e2fsprogs make use of it.

As mentioned inline - I would suggest sticking to dlopen, in part because forcing extra deps isn't fun. And in part because the cmake libarchive build produces a different soname - see this bug.

The cmake build does not produce a pkg-config file, which in part why I would suggest using the PKG_CHECK_MODULES macro.

HTH o/

[evelikov]

I would swap the above check for a simple PKG_CHECK_MODULES(ARCHIVE, [libarchive >= the-version-whose-api-is-used.

Since this is an optional feature, I would only have the dlopen path. Using linking and forcing bunch of runtime dependencies onto people who won't use the feature seems moot IMHO.

[evelikov]

Our of curiosity: where does the 16MiB come from? It's few orders of magnitude larger than the minimum 64KiB.

[josch]

From Andreas Dilger, see https://lore.kernel.org/linux-ext4/1FD4874D-0E9C-442C-9FC1-AC35DCFD0A3C@dilger.ca/

[evelikov]

Might be worth adding an inline comment/reference IMHO.

[richardweinberger]

FWIW, I really would love to see this feature merged.

[josch]

Hello and happy new year! 🙂

I just rebased this branch on top of master. If there is anything I can do to move this forward, @tytso, please do tell. Thank you!

[josch]

The github workflow seems to fail for macos and android but the failures do not seem to be due to the changes of this merge request. In fact on android, the relevant tests are successfully skipped:

2024-01-09T11:58:40.7151560Z m_rootgnutar: create fs image from GNU tarball: skipped (no libarchive)
2024-01-09T11:58:40.7473800Z m_roottar: skipped (no libarchive)
2024-01-09T11:58:40.7475090Z m_rootpaxtar: skipped (no libarchive)

[h01ger]

hey, any update on this? being able to create reproducible ext3/4 filesystems would be really great.

[tytso]

Apologies for the delay in reviewing it. Things have just been really crazy. I am in the middle of reviewing the patch right now. One thing which I've noticed is that the test m_rootgnutar assumes that "tar" is the GNU tar. This might not be true on non-Linux platforms for which e2fprogs is built (e.g., *BSD, MacOS, etc.). So it might be that we need a mkgnutar.pl ala the m_roottar and m_rootpaxtar tests.

[josch]

Thanks a lot for reviewing my code! 🥳 I'm currently in the middle of switching jobs, so I cannot promise you until when I can cook up a mkgnutar.pl. Maybe a quicker fix would be to check for gnu tar and skip the test if it's not available for now?

EDIT

Nevermind. Replicating what GNU tar does and creating bit-by-bit identical tarballs with a Perl script was easy enough. @tytso do you want me to throw out GNU tar completely or do you think that there is value to have two tests: one which is only run if GNU tar is available and one which does the same thing but with a Perl fake tar.

[josch]

I now replaced all calls to tar in m_rootgnutar by mkgnutar.pl. I didn't rebase on master yet, so that you can easily compare the changes of my last force-push. In the process I also found a bug in the GNU tar version, hence the changes to the expect file.

[tytso]

OK, things looks good. I've done a trial merge with the next branch, and the CI passes clean:

https://github.com/tytso/e2fsprogs/actions/runs/8743318976

Do you have any other changes you want to make? If not, I can just take the merge at https://github.com/tytso/e2fsprogs/tree/josch-libarchive

[josch]

If there are more things that I want to change/fix, you will probably receive pull requests after the first e2fsprogs version with this feature lands in Debian unstable and I start using this functionality in all the places I plan to us it (like mmdebstrap, debvm, or the MNT Reform system image tooling).

If there are any bugs related to this functionality, please do not hesitate to ping me about it either via github or via josch@debian.org.

Thank you!! ❤️

[tytso]

I've merged this on the next branch, and have noted a potential portability problem with the m_root*tar test scripts when I tried doing a test build on FreeBSD 14. Namely, the -c option to stat is something which is only in the GNU coreutils implementation. It is not in the version of stat found in the *BSD userspace, which is also going to be the version found in MacOS/Darwin (because it is based on FreeBSD's userspace). Fortunately, MacOS apparently doesn't ship with libarchive, so it's not breaking the CI test on github. Since the test scripts are using perl, probably what needs to happen is implement "stat -c %Y" in perl.

stat: illegal option -- c
usage: stat [-FLnq] [-f format | -l | -r | -s | -x] [-t timefmt] [file|handle ...]
Value "" invalid for option mtime (number expected)
Use of uninitialized value $mtime in sprintf at m_rootgnutar/mkgnutar.pl line 73.
Use of uninitialized value $mtime in sprintf at m_rootgnutar/mkgnutar.pl line 73.
Use of uninitialized value $mtime in sprintf at m_rootgnutar/mkgnutar.pl line 73.

If you have time to fix this, that would be great. If not, I'm sure that once I do an e2fsprogs release, the FreeBSD ports maintainer for e2fsprogs will send a patcch fairly quickly once it is released. :-)

[josch]

Since the test scripts are using perl, probably what needs to happen is implement "stat -c %Y" in perl.

$ stat -c %Y testfile
1706820340
$ perl -e 'print((stat ($ARGV[0]))[9])' testfile
1706820340

I can also send you a full patch but only later as it is currently Sunday morning over here. Thank you for notifying me about this and thank you for merging! :)

EDIT: sent you a patch by mail