Code clean-up, proper CMake integrationfeat/tdeio-xattr-support

Signed-off-by: Alex Kent Hajnal <software@alephnull.net>

5 files changed, 111 insertions, 52 deletions

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 84158ef6d..3618f20b7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -284,13 +284,17 @@ if( NOT HAVE_SYS_UCRED_H )
   endif( )
 endif( )
 check_include_file( "sys/xattr.h" HAVE_SYS_XATTR_H )
+
 check_include_file( "attr/libattr.h" HAVE_ATTR_LIBATTR_H)
 if ( HAVE_ATTR_LIBATTR_H )
-    check_library_exists( attr attr_copy_file "" HAVE_ATTR__ATTR_COPY_FILE )
-endif( )
-if( HAVE_ATTR__ATTR_COPY_FILE )
-    set( DO_USER_XATTR_COPYING "1" )
+  message( STATUS "Have attr/libattr.h" )
+  check_library_exists( attr attr_copy_file "" HAVE_ATTR__ATTR_COPY_FILE )
+  if( HAVE_ATTR__ATTR_COPY_FILE )
+    message( STATUS "Have attr_copy_file in libattr" )
+    set( USER_XATTR_LIBRARIES attr )
+    set( DO_USER_XATTR_COPYING 1 )
     message( STATUS "User extended attribute copy support enabled" )
+  endif( )
 endif( )
 
 check_include_file( "termios.h" HAVE_TERMIOS_H )
diff --git a/tdeio/CMakeLists.txt b/tdeio/CMakeLists.txt
index 26bfc53aa..2df71788e 100644
--- a/tdeio/CMakeLists.txt
+++ b/tdeio/CMakeLists.txt
@@ -92,6 +92,6 @@ tde_add_library( ${target} SHARED
   VERSION 14.0.0
   EMBED kssl-static tdeiocore-static tdesycoca-static kbookmarks-static tdefile-static ${ELFICON_STATIC_LIB}
   LINK tdeui-shared tdewalletclient-shared
-  LINK_PRIVATE ltdlc-static tdesu-shared ${LIBR_LIBRARIES} ${ACL_LIBRARIES}
+  LINK_PRIVATE ltdlc-static tdesu-shared ${LIBR_LIBRARIES} ${ACL_LIBRARIES} ${USER_XATTR_LIBRARIES}
   DESTINATION ${LIB_INSTALL_DIR}
 )
diff --git a/tdeio/tdeio/CMakeLists.txt b/tdeio/tdeio/CMakeLists.txt
index 91828320a..621101c48 100644
--- a/tdeio/tdeio/CMakeLists.txt
+++ b/tdeio/tdeio/CMakeLists.txt
@@ -84,6 +84,13 @@ tde_add_library( ${target} STATIC_PIC AUTOMOC
   SOURCES ${${target}_SRCS}
 )
 
+if( DO_USER_XATTR_COPYING )
+  add_compile_definitions( DO_USER_XATTR_COPYING )
+  if ( HAVE_ATTR__ATTR_COPY_FILE )
+    add_compile_definitions( HAVE_ATTR__ATTR_COPY_FILE )
+  endif ( HAVE_ATTR__ATTR_COPY_FILE )
+endif( DO_USER_XATTR_COPYING )
+
 
 ##### tdesycoca ###################################
 
diff --git a/tdeio/tdeio/README.xattr b/tdeio/tdeio/README.xattr
index 50dcaf42b..0c7d32d21 100644
--- a/tdeio/tdeio/README.xattr
+++ b/tdeio/tdeio/README.xattr
@@ -9,22 +9,13 @@ source file has been closed and immediately before the source file is deleted
 (in the case that a move is being done).  This code is based on the sample 
 code[4] provided with libattr (examples/copyattr.c).
 
-This code has been in daily use by the author of this patch for just over five 
-years without any issues.  What's missing though is the addition of tests for 
-xattr and libattr support at configuration/compile time.  The code I've added 
-is enclosed by '#ifdef DO_USER_XATTR_COPYING' statements so a test should be 
-added that sets this macro[5] if configure-time tests show that xattrs/libattr 
-are working.  I've added a preliminary test to CMakeLists.txt but I'm not sure 
-it's correct.  Alternatively uncomment the '#define DO_USER_XATTR_COPYING 1' 
-statement at the top of the code to force my code to be enabled.  Ensuring that 
-the cmake test working correctly needs to be done before general release.
-
 This code has been tested on select Ubuntu Linux releases (natty, precise, 
 xenial, and jammy) using x86_64 Linux kernels 3.14.1, 4.18.3, and 5.15.0.
-Tested filesystems are ext4 and squashfs.
+Tested filesystems are ext4 and squashfs.  For ext2/3/4 filesytems the ext_attr 
+feature must be enabled and the filesystem mounted with the user_xattr option.
 
 As far as cross-platform availability goes this code is only known to work on 
-Linux[6][7].  That said, this code adds functionality that (as far as I'm aware) 
+Linux[5][6].  That said, this code adds functionality that (as far as I'm aware) 
 isn't currently present in TDE on any of the platforms it runs on.  In other 
 words, even if it only runs on Linux in its current state there is no loss of 
 functionality on any other platforms (just a lack of a gaining a feature).  In 
@@ -32,7 +23,7 @@ addition, determining the proper location to insert the library call was the
 hard part with the actual implementation being quite straightforward.  Adding 
 additional backends for other platforms should be pretty easy.
 
-To test the code build and install it[8] then run the following from the 
+To test the code build and install it[7] then run the following from the 
 commandline in e.g. Konsole:
 
   $ cd PATH_SUPPORTING_USER_XATTRS_AT_THE_FILESYSTEM_LEVEL
@@ -71,17 +62,16 @@ Enjoy!
 
 [4] The code that this patch is based on is licensed GPL v2 (or later).
 
-[5] One has to use '#define DO_USER_XATTR_COPYING 1'; specifying instead  
-   '#define DO_USER_XATTR_COPYING' doesn't enable the code (at least on my 
-   system).
-
-[6] I'm not sure which non-Linux platforms (if any) libattr runs on.
+[5] I'm not sure which non-Linux platforms (if any) runs on.  It appears that 
+    Cygwin and GNU/Hurd are also supported but this has not been verified.
 
-[7] Another known issue is namespaces.  Linux, FreeBSD, and NetBSD support 
+[6] One potential issue is namespaces.  Linux, FreeBSD, and NetBSD support 
     xattrs using the 'user' namespace.  Solaris and OSX support user xattrs 
-    but do not support namespaces.
+    but do not support namespaces.  When using libattr, attributes from all 
+    namespaces are copied except those whose names match the patterns listed 
+    in /etc/xattr.conf; other platforms may need special handling.
     (source: File::ExtAttr(3pm), personal testing)
 
-[8] Instead of doing a full install one can simply copy the updated library:
+[7] Instead of doing a full install one can simply copy the updated library:
     
     cp -va "BUILD_DIR/tdeio/libtdeio.so.14.0.0" /opt/trinity/lib/ && ldconfig
diff --git a/tdeio/tdeio/job.cpp b/tdeio/tdeio/job.cpp
index 48cc68e3b..726691d27 100644
--- a/tdeio/tdeio/job.cpp
+++ b/tdeio/tdeio/job.cpp
@@ -95,10 +95,12 @@ extern "C" {
 
 #ifdef DO_USER_XATTR_COPYING
 
-//#include <stdio.h>  // For stderr, (v)fprintf (already included above)
+#ifdef HAVE_ATTR__ATTR_COPY_FILE
+
+//#include <stdio.h>  // For stderr, vsnprintf (already included above)
 //#include <errno.h>  // For errno (already included above)
 //#include <stdlib.h> // For free (already included above)
-//#include <string.h> // For strerror, strdup, and strcmp (already implicitly included above)
+//#include <string.h> // For strerror, strdup, and strncmp (already implicitly included above)
 #include <stdarg.h> // For va_start and va_end
 
 extern "C" {
@@ -110,17 +112,53 @@ extern "C" {
  * Optional error handler for attr_copy_file(). CTX is the error
  * context passed to attr_copy_file(), ERR is the errno value
  * that occurred. FMT and the rest are printf style arguments.
+ * 
+ * No action is taken beyond logging.  Errors commonly occur when copying 
+ * files to filesystems that don't support extended attributes (such as 
+ * /tmp on Linux).
  */
 static void
 error(struct error_context *ctx, const char *fmt, ...)
 {
-	va_list ap;
-
-	va_start(ap, fmt);
-	if (vfprintf(stderr, fmt, ap))
-		fprintf(stderr, ": ");
-	fprintf(stderr, "%s\n", strerror(errno));
-	va_end(ap);
+//  Original sample code:
+//  va_list ap;
+//
+//  va_start(ap, fmt);
+//  if (vfprintf(stderr, fmt, ap))
+//    fprintf(stderr, ": ");
+//  fprintf(stderr, "%s\n", strerror(errno));
+//  va_end(ap);
+  
+  va_list ap;
+  
+  // Save a copy in case it gets clobbered (shouldn't be needed)
+  int our_errno = errno;
+  
+  // Get length of formatted string (may be 0)
+  va_start(ap, fmt);
+  int len = vsnprintf(nullptr, 0, fmt, ap);
+  va_end(ap);
+  
+  if ( len ) {
+    // There's a prefix string, output it
+    char* buffer = (char*)malloc(len+1);
+    if ( buffer ) {
+      // Buffer allocated, write the error to it
+      va_start(ap, fmt);
+      vsnprintf(&buffer[0], len+1, fmt, ap);
+      va_end(ap);
+      // Write message to log
+      kdDebug(7007) << "Error in attr_copy_file: " << buffer << ": " << strerror(our_errno) << endl;
+    } else {
+      // Buffer allocation failed, just log what we can
+      kdDebug(7007) << "Error in attr_copy_file: " << strerror(our_errno) << endl;
+    }
+    free(buffer);
+    
+  } else {
+    // No prefix string, just log the error code
+    kdDebug(7007) << "Error in attr_copy_file: " << strerror(our_errno) << endl;
+  }
 }
 
 /*
@@ -130,18 +168,18 @@ error(struct error_context *ctx, const char *fmt, ...)
 static const char *
 quote(struct error_context *ctx, const char *pathname)
 {
-	char *pn = strdup(pathname), *p;
-	pathname = strdup(pathname);
-	for (p = pn; *p != '\0'; p++)
-		if (*p & 0x80)
-			*p='?';
-	return pn;
+  char *pn = strdup(pathname), *p;
+  pathname = strdup(pathname);
+  for (p = pn; *p != '\0'; p++)
+    if (*p & 0x80)
+      *p='?';
+  return pn;
 }
 
 static void
 quote_free(struct error_context *ctx, const char *name)
 {
-	free((void *)name);
+  free((void *)name);
 }
 
 /*
@@ -150,20 +188,33 @@ quote_free(struct error_context *ctx, const char *name)
 struct error_context ctx = { error, quote, quote_free };
 
 /*
- * Optional attribute filter for attr_copy_file(). This example
- * excludes all attributes other than extended user attributes.
+ * Optional attribute filter for attr_copy_file(). This example 
+ * excludes all attributes other than extended user attributes 
+ * (those prefixed with "user.").
+ * 
+ * Returns 0 if the attribute should be copied, non-zero otherwise
+ * 
+ * If this function is not specified then the default behaviour of 
+ * attr_copy_file is to copy all attributes except for ACLs (as listed
+ * in /etc/xattr.conf; all patterns that match are skipped).
  * 
  * This function isn't used by the current code.
  */
 static int is_user_attr(const char *name, struct error_context *ctx)
 {
-	// Note that non-Linux systems (including Solaris and Darwin/OSX) may 
-	// use a different naming scheme for user attributes.  AFAIK FreeBSD 
-	// and NetBSD do support the "user" namespace.
-	return strcmp(name, "user.") == 0;
+  // Note that non-Linux systems (including Solaris and Darwin/OSX) may 
+  // use a different naming scheme for user attributes.  AFAIK FreeBSD 
+  // and NetBSD do support the "user" namespace.
+  return strncmp(name, "user.", 5) == 0;
+  
+  // If this function is not specified in the call to attr_copy_file then the 
+  // default behavior is to use the following heuristic instead (see above):
+  // return attr_copy_action(name, ctx) == 0;
 }
 
-#endif /* USE_XATTR */
+#endif /* HAVE_ATTR__ATTR_COPY_FILE */
+
+#endif /* DO_USER_XATTR_COPYING */
 
 
 
@@ -2049,11 +2100,18 @@ void FileCopyJob::slotResult( TDEIO::Job *job)
       m_copyJob = 0;
 
 #ifdef DO_USER_XATTR_COPYING
+#ifdef HAVE_ATTR__ATTR_COPY_FILE
       {
-          // Copy extended attributes (added AKH 2017-11-26)
-          attr_copy_file(TQFile::encodeName(m_src.path()), TQFile::encodeName(m_dest.path()), NULL, &ctx);
+         // Copy extended attributes (added AKH 2017-11-26)
+         // Returned error code (0 on success, -1 on failure) is ignored
+         int err = attr_copy_file(TQFile::encodeName(m_src.path()), TQFile::encodeName(m_dest.path()), NULL, &ctx);
+         
+         // The same but using the attribute name filter function defined above (is_user_attr)
+         //int err = attr_copy_file(TQFile::encodeName(m_src.path()), TQFile::encodeName(m_dest.path()), is_user_attr, &ctx);
       }
-#endif
+
+#endif /* HAVE_ATTR__ATTR_COPY_FILE */
+#endif /* DO_USER_XATTR_COPYING */
 
       if (m_move)
       {