Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Johannes Schauer Marin Rodrigues <josch@mister-muffin.de>
To:     linux-ext4@vger.kernel.org
Cc:     Johannes Schauer Marin Rodrigues <josch@mister-muffin.de>,
        adilger@dilger.ca, djwong@kernel.org
Subject: [PATCH v2 0/1] mke2fs: the -d option can now handle tarball input
Date:   Sat, 12 Aug 2023 17:02:04 +0200
Message-Id: <20230812150204.462962-1-josch@mister-muffin.de>
In-Reply-To: 605DCBDE-A388-4B98-BF5A-38773F15E3F4@dilger.ca
References: <605DCBDE-A388-4B98-BF5A-38773F15E3F4@dilger.ca>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Andreas,

thank you for your input!

> Having just looked through the patch, I think it could use some cleanup.
> Basic code style issues:
> - wrapping lines at 80-columns
> - avoid use of C++ comments in the code
> - consistent indentation for continued lines
> - consistent one blank line between functions
> - consistent one blank line after variable declarations

I've used clang-format with the `.clang-format` from the linux git to
format the code. I also ran `scripts/checkpatch.pl` from linux to fix
some more issues.

> - split large highly-indented code blocks into helper functions

There was particularly one highly indented switch() which I now put into
its own function.

> In terms of code structure, refactoring it to put libarchive handling 
> in a separate file (e.g. mke2fs-archive.c or similar) would also make 
> the maintenance easier, since it can be added/removed from the build 
> more easily, and (if necessary) removed from the tree if it is no longer 
> working.

I've moved most of the libarchive functions into misc/create_inode_libarchive.c
and hope this improves the situation. I've had to modify a couple of
Makefile.in, make some functions of misc/create_inode.c non-extern and add them
to create_inode.h so that misc/create_inode_libarchive.c can make use of them.
Is this what you had in mind?

> Then have only a couple of small function calls in the main mke2fs.c 
> code that are accessing the libarchive functionality if it is built-in, 
> or being no-ops (or just printing the error message) if libarchive is 
> unavailable.

All the libarchive-specific functionality is behind __populate_fs_from_tar()
which is the only function exposed in misc/create_inode_libarchive.h. If
e2fsprogs was compiled without libarchive, an error message will be shown
if the user tries to pass a regular file instead of a directory. If
e2fsprogs was compiled with libarchive but the user does not have the
shared library installed, another error about this will be displayed.

There are probably still many things about my patch that can be improved.

Thanks a lot for considering this and having a look at it!

cheers, josch


P.S. (more like a record for future-me) I tested that my changes allow
libarchive to be compiled with and without libarchive headers installed
and with and without libarchive shared library installed by running:

mmdebstrap --variant=apt --include=git,ca-certificates,build-essential,autoconf,automake,autoconf-archive,pkg-config,gettext,texinfo,libblkid-dev,uuid-dev,m4,libarchive-dev \
	--chrooted-customize-hook='git clone https://github.com/josch/e2fsprogs.git 
		&& cd e2fsprogs && git checkout libarchive-linux-ext4 && autoreconf -fi
		&& ./configure && make -j4
		&& make check || cat tests/m_rootgnutar.failed tests/m_rootgnutar.log
		&& apt remove --yes libarchive13
		&& tar -C include -c . | ./misc/mke2fs -q -F -o Linux -T ext4 -O metadata_csum,64bit -E lazy_itable_init=1 -b 1024 -d - image.ext4 16384' \
	unstable /dev/null


Johannes Schauer Marin Rodrigues (1):
  mke2fs: the -d option can now handle tarball input

 MCONFIG.in                     |   1 +
 configure                      | 134 ++++---
 configure.ac                   |   9 +
 debugfs/Makefile.in            |  25 +-
 lib/config.h.in                |   3 +
 lib/ext2fs/Makefile.in         |  25 +-
 misc/Makefile.in               |  17 +-
 misc/create_inode.c            |  61 ++-
 misc/create_inode.h            |  10 +
 misc/create_inode_libarchive.c | 677 +++++++++++++++++++++++++++++++++
 misc/create_inode_libarchive.h |  10 +
 misc/mke2fs.8.in               |  10 +-
 misc/mke2fs.c                  |  12 +-
 tests/m_rootgnutar/expect      | 141 +++++++
 tests/m_rootgnutar/output.sed  |   5 +
 tests/m_rootgnutar/script      | 123 ++++++
 tests/m_rootpaxtar/expect      |  87 +++++
 tests/m_rootpaxtar/mkpaxtar.pl |  69 ++++
 tests/m_rootpaxtar/output.sed  |   5 +
 tests/m_rootpaxtar/script      |  44 +++
 tests/m_roottar/expect         | 208 ++++++++++
 tests/m_roottar/mktar.pl       |  62 +++
 tests/m_roottar/output.sed     |   5 +
 tests/m_roottar/script         |  57 +++
 24 files changed, 1714 insertions(+), 86 deletions(-)
 create mode 100644 misc/create_inode_libarchive.c
 create mode 100644 misc/create_inode_libarchive.h
 create mode 100644 tests/m_rootgnutar/expect
 create mode 100644 tests/m_rootgnutar/output.sed
 create mode 100644 tests/m_rootgnutar/script
 create mode 100644 tests/m_rootpaxtar/expect
 create mode 100644 tests/m_rootpaxtar/mkpaxtar.pl
 create mode 100644 tests/m_rootpaxtar/output.sed
 create mode 100644 tests/m_rootpaxtar/script
 create mode 100644 tests/m_roottar/expect
 create mode 100644 tests/m_roottar/mktar.pl
 create mode 100644 tests/m_roottar/output.sed
 create mode 100644 tests/m_roottar/script

-- 
2.40.0