Hi Ted, Quoting Theodore Ts'o (2024-12-02 02:22:19) > Your patch didn't quite work correctly; if archive.h is not present, the > build will not fail; but it won't atually try to use dlopen() to access the > libarchive.so dynamic library if it happens to be present. > > That does't really matter in the bootstraping case, but it *does* mean > that if the goal is to get a fully-functional e2fsprogs package, you > will need to rebuild it after libarchive-dev has been created. And if > this was considered sufficient, you wouldn't need this patch at all; > it would be sufficient to just have a <!nocheck> added to the > libarchive-dev depedency. > > Speaking of which, this patch doesn't doesn't have the change to > debian/control to add <!nocheck>.
I was working on the upstream git repo and didn't realize that that repository also includes a ./debian directory. Fixed, thanks! > I believe this if your patch is supplemented with these two changes, it > should do the right thing. Do you agree? I think you are right and my previous patch had a bug. Your changes fix it but it also made me realize how the current #ifdef preprocessor logic in that file is quite confusing. I think by refactoring it a bit and duplicating the signature of __populate_fs_from_tar(), it can be made much more readable. So attached patch organizes the code a bit differently. At the top, CONFIG_DISABLE_LIBARCHIVE is checked and __populate_fs_from_tar is made to just return an error in that case. The #else branch is then the complete remainder of the file. I think by avoiding to interleave the preprocessor conditions, it becomes much more obvious which blocks are enabled or disabled depending on the setting of CONFIG_DISABLE_LIBARCHIVE. I also added a bunch more code comments as I was a bit struggling to remember all the bit and pieces surrounding these changes. I think the refactoring and comments will make working with that file easier in the future. What do you think? I sent the patch to Helmut for testing but gcc-14 currently FTBFS in unstable and we have to wait with testing my patch in rebootstrap until that works again. But if you have further remarks on my patch, maybe I can use the time until gcc is fixed to get the patch into a state that is acceptable for you. Thanks! cheers, josch
>From 27096e7046a9c545e970f7580fc657cc604fce85 Mon Sep 17 00:00:00 2001
From: Johannes Schauer Marin Rodrigues <jo...@mister-muffin.de>
Date: Tue, 3 Sep 2024 13:15:33 +0200
Subject: [PATCH] misc/create_inode_libarchive.c: decouple --without-libarchive
and HAVE_ARCHIVE_H
To aid bootstrapping, it would be useful to be able to build e2fsprogs
without archive.h as otherwise there is a build dependency loop with
libarchive. If archive.h is not present, add the missing forward
declarations (as opaque structs) and preprocessor definitions and
typedefs. Since this allows building e2fsprogs with libarchive support
even without the archive.h header present on the system, we cannot check
HAVE_ARCHIVE_H anymore to decide whether to build with libarchive
support or not. So if --without-libarchive is passed to ./configure,
CONFIG_DISABLE_LIBARCHIVE gets set and later checked to decide about
libarchive support.
Closes: #1078693
---
configure | 3 +++
configure.ac | 2 ++
debian/control | 2 +-
lib/config.h.in | 3 +++
misc/create_inode_libarchive.c | 46 ++++++++++++++++++++++++++--------
5 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/configure b/configure
index cba3191c..3310cecc 100755
--- a/configure
+++ b/configure
@@ -13745,6 +13745,9 @@ then
try_libarchive=""
{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: Disabling libarchive support" >&5
printf "%s\n" "Disabling libarchive support" >&6; }
+
+printf "%s\n" "#define CONFIG_DISABLE_LIBARCHIVE 1" >>confdefs.h
+
elif test "$withval" = "direct"
then
try_libarchive="direct"
diff --git a/configure.ac b/configure.ac
index 131caef3..ed5ba0aa 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1307,6 +1307,8 @@ AS_HELP_STRING([--without-libarchive],[disable use of libarchive]),
then
try_libarchive=""
AC_MSG_RESULT([Disabling libarchive support])
+ AC_DEFINE(CONFIG_DISABLE_LIBARCHIVE, 1,
+ [Define to 1 to completely disable libarchive])
elif test "$withval" = "direct"
then
try_libarchive="direct"
diff --git a/debian/control b/debian/control
index e641b371..d33a59dd 100644
--- a/debian/control
+++ b/debian/control
@@ -2,7 +2,7 @@ Source: e2fsprogs
Section: admin
Priority: required
Maintainer: Theodore Y. Ts'o <ty...@mit.edu>
-Build-Depends: dpkg-dev (>= 1.22.5), gettext, texinfo, pkgconf, libarchive-dev, libfuse3-dev [linux-any kfreebsd-any] <!pkg.e2fsprogs.no-fuse2fs>, debhelper-compat (= 12), dh-exec, libblkid-dev, uuid-dev, m4, udev [linux-any], systemd [linux-any], systemd-dev [linux-any], cron [linux-any], dh-sequence-movetousr
+Build-Depends: dpkg-dev (>= 1.22.5), gettext, texinfo, pkgconf, libarchive-dev <!nocheck>, libfuse3-dev [linux-any kfreebsd-any] <!pkg.e2fsprogs.no-fuse2fs>, debhelper-compat (= 12), dh-exec, libblkid-dev, uuid-dev, m4, udev [linux-any], systemd [linux-any], systemd-dev [linux-any], cron [linux-any], dh-sequence-movetousr
Rules-Requires-Root: no
Standards-Version: 4.7.0
Homepage: http://e2fsprogs.sourceforge.net
diff --git a/lib/config.h.in b/lib/config.h.in
index 04cec72b..819c4331 100644
--- a/lib/config.h.in
+++ b/lib/config.h.in
@@ -12,6 +12,9 @@
/* Define to 1 for features for use by ext4 developers */
#undef CONFIG_DEVELOPER_FEATURES
+/* Define to 1 to completely disable libarchive */
+#undef CONFIG_DISABLE_LIBARCHIVE
+
/* Define to 1 if using dlopen to access libarchive */
#undef CONFIG_DLOPEN_LIBARCHIVE
diff --git a/misc/create_inode_libarchive.c b/misc/create_inode_libarchive.c
index 8705eb16..f38d6616 100644
--- a/misc/create_inode_libarchive.c
+++ b/misc/create_inode_libarchive.c
@@ -13,20 +13,50 @@
#define _GNU_SOURCE 1
#include "config.h"
-#include <ext2fs/ext2_types.h>
#include "create_inode.h"
#include "create_inode_libarchive.h"
#include "support/nls-enable.h"
-#ifdef HAVE_ARCHIVE_H
+#ifdef CONFIG_DISABLE_LIBARCHIVE
+
+/* If ./configure was run with --without-libarchive, then only
+ * __populate_fs_from_tar() remains in this file and will return an error. */
+errcode_t __populate_fs_from_tar(ext2_filsys, ext2_ino_t, const char *,
+ ext2_ino_t, struct hdlinks_s *,
+ struct file_info *,
+ struct fs_ops_callbacks *) {
+ com_err(__func__, 0,
+ _("you need to compile e2fsprogs without --without-libarchive"
+ "be able to process tarballs"));
+ return 1;
+}
+
+#else
+
+/* If ./configure was NOT run with --without-libarchive, then build with
+ * support for dlopen()-ing libarchive at runtime. This will also work even
+ * if archive.h is not available at compile-time. See the comment below. */
/* 64KiB is the minimum blksize to best minimize system call overhead. */
//#define COPY_FILE_BUFLEN 65536
//#define COPY_FILE_BUFLEN 1048576
#define COPY_FILE_BUFLEN 16777216
+/* If archive.h was found, include it as usual. To support easier
+ * bootstrapping, also allow compilation without archive.h present by
+ * declaring the necessary opaque structs and preprocessor definitions. */
+#ifdef HAVE_ARCHIVE_H
#include <archive.h>
#include <archive_entry.h>
+#else
+struct archive;
+struct archive_entry;
+#define ARCHIVE_EOF 1 /* Found end of archive. */
+#define ARCHIVE_OK 0 /* Operation was successful. */
+#include <unistd.h> /* ssize_t */
+typedef ssize_t la_ssize_t;
+#endif /* HAVE_ARCHIVE_H */
+
#include <libgen.h>
#include <locale.h>
@@ -175,7 +205,7 @@ static int libarchive_available(void)
return 1;
}
-#endif
+#endif /* CONFIG_DLOPEN_LIBARCHIVE */
static errcode_t __find_path(ext2_filsys fs, ext2_ino_t root, const char *name,
ext2_ino_t *inode)
@@ -541,7 +571,6 @@ static errcode_t handle_entry(ext2_filsys fs, ext2_ino_t root_ino,
}
return 0;
}
-#endif
errcode_t __populate_fs_from_tar(ext2_filsys fs, ext2_ino_t root_ino,
const char *source_tar, ext2_ino_t root,
@@ -549,12 +578,6 @@ errcode_t __populate_fs_from_tar(ext2_filsys fs, ext2_ino_t root_ino,
struct file_info *target,
struct fs_ops_callbacks *fs_callbacks)
{
-#ifndef HAVE_ARCHIVE_H
- com_err(__func__, 0,
- _("you need to compile e2fsprogs with libarchive to "
- "be able to process tarballs"));
- return 1;
-#else
char *path2, *path3, *dir, *name;
unsigned int dir_exists;
struct archive *a;
@@ -700,5 +723,6 @@ out:
uselocale(old_locale);
freelocale(archive_locale);
return retval;
-#endif
}
+
+#endif /* CONFIG_DISABLE_LIBARCHIVE */
--
2.39.2
signature.asc
Description: signature
