Code clean-up, proper CMake integration

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

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 )
+  message( STATUS "Have attr/libattr.h" )
   check_library_exists( attr attr_copy_file "" HAVE_ATTR__ATTR_COPY_FILE )
-endif( )
+  if( HAVE_ATTR__ATTR_COPY_FILE )
-if( HAVE_ATTR__ATTR_COPY_FILE )
+    message( STATUS "Have attr_copy_file in libattr" )
-    set( DO_USER_XATTR_COPYING "1" )
+    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 )

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}
 )

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 ###################################
 

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  
+[5] I'm not sure which non-Linux platforms (if any) runs on.  It appears that 
-   '#define DO_USER_XATTR_COPYING' doesn't enable the code (at least on my 
+    Cygwin and GNU/Hurd are also supported but this has not been verified.
-   system).
-
-[6] I'm not sure which non-Linux platforms (if any) libattr runs on.
 
-[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

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, ...)
 {
+//  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);
-	if (vfprintf(stderr, fmt, ap))
+      vsnprintf(&buffer[0], len+1, fmt, ap);
-		fprintf(stderr, ": ");
-	fprintf(stderr, "%s\n", strerror(errno));
       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;
+  }
 }
 
 /*
@@ -151,7 +189,14 @@ 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.
+ * 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.
  */
@@ -160,10 +205,16 @@ 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;
+  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);
+         // 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)
       {