Re: [Bug-tar] [PATCH] Fix build with xattr support

[Pavel Raiskup]

i Anthony, thanks for looking at it!  I haven't test it, but here is
"static" analysis:

> diff --git a/acinclude.m4 b/acinclude.m4
> index d48c881..18cfd49 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -37,18 +37,40 @@ AC_DEFUN([TAR_HEADERS_ATTR_XATTR_H],
>      [], [with_xattrs=maybe]
>    )
>  
> -  AC_CHECK_HEADERS([attr/xattr.h])
> -  AM_CONDITIONAL([TAR_COND_XATTR_H],[test "$ac_cv_header_attr_xattr_h" = 
> yes])
> -  if test "$ac_cv_header_attr_xattr_h" = yes; then
> +  # First check for <sys/xattr.h>
> +  AC_CHECK_HEADERS([sys/xattr.h])
> +  AM_CONDITIONAL([TAR_COND_XATTR_H],[test "$ac_cv_header_sys_xattr_h" = yes])
> +  AM_CONDITIONAL([TAR_LIB_ATTR],[false])
> +  if test "$ac_cv_header_sys_xattr_h" = yes; then
>      AC_CHECK_FUNCS(getxattr  fgetxattr  lgetxattr \
>                     setxattr  fsetxattr  lsetxattr \
>                     listxattr flistxattr llistxattr,
>          # only when functions are present
> -        AC_DEFINE([HAVE_ATTR_XATTR_H], [1],
> -                    [define to 1 if we have <attr/xattr.h> header])
> +        AC_DEFINE([HAVE_SYS_XATTR_H], [1],
> +                    [define to 1 if we have <sys/xattr.h> header])
>          if test "$with_xattrs" != no; then
>            AC_DEFINE([HAVE_XATTRS],,[Define when we have working linux 
> xattrs.])
>          fi
>      )
>    fi

This part seems to be OK.

> +  # If <sys/xattr.h> is not found, then check for <attr/xattr.h>
> +  if test "$ac_cv_header_sys_xattr_h" != yes; then
> +    AC_CHECK_HEADERS([attr/xattr.h])
> +    AM_CONDITIONAL([TAR_COND_XATTR_H],[test "$ac_cv_header_attr_xattr_h" = 
> yes])
> +    AC_CHECK_LIB([attr],[fgetxattr])
> +    AM_CONDITIONAL([TAR_LIB_ATTR],[test "$ac_cv_lib_attr_fgetxattr" = yes])
> +    if test "$ac_cv_header_attr_xattr_h" = yes; then
> +      AC_CHECK_FUNCS(getxattr  fgetxattr  lgetxattr \
> +                     setxattr  fsetxattr  lsetxattr \
> +                     listxattr flistxattr llistxattr,
> +          # only when functions are present
> +          AC_DEFINE([HAVE_ATTR_XATTR_H], [1],
> +                      [define to 1 if we have <attr/xattr.h> header])
> +          if test "$with_xattrs" != no; then
> +            AC_DEFINE([HAVE_XATTRS],,[Define when we have working linux 
> xattrs.])
> +          fi
> +      )
> +    fi
> +  fi
>  ])

This is IMO not going to work.  AC_CHECK_FUNCS will not success if you
don't have libc with xattrs support;  you should use AC_SEARCH_LIBS (in
'attr' library).  Look at configure.ac and acl-related checks.

I also don't like the functions to have listed twice..  Would it be
possible to have at least the list of functions defined at one place?

> diff --git a/lib/xattr-at.h b/lib/xattr-at.h
> index 2981771..1f517d0 100644
> --- a/lib/xattr-at.h
> +++ b/lib/xattr-at.h
> @@ -20,7 +20,15 @@
>  #define XATTRS_AT_H
>  
>  #include <sys/types.h>
> -#include <attr/xattr.h>
> +#if defined(HAVE_SYS_XATTR_H)
> +# include <sys/xattr.h>
> +#elif defined(HAVE_ATTR_XATTR_H)
> +# include <attr/xattr.h>
> +#endif
> +
> +#ifndef ENOATTR
> +# define ENOATTR ENODATA        /* No such attribute */
> +#endif
>  
>  /* These are the dir-fd-relative variants of the functions without the
>     "at" suffix.  For example, setxattrat (AT_FDCWD, path, name, value, size,
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 07c117d..5e2f330 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -51,3 +51,7 @@ AM_CFLAGS = $(WARN_CFLAGS) $(WERROR_CFLAGS)
>  LDADD = ../lib/libtar.a ../gnu/libgnu.a $(LIBINTL) $(LIBICONV)
>  
>  tar_LDADD = $(LIBS) $(LDADD) $(LIB_CLOCK_GETTIME) $(LIB_EACCESS) 
> $(LIB_SELINUX)
> +
> +if TAR_LIB_ATTR
> +tar_LDADD += -lattr
> +endif

Also, I would say the AM_CONDITIONAL is not needed.  The LIBS should be
automatically adjusted by AC_SEARCH_LIBS.

Note that we are still linking *xattr() functions against glibc even if
the tar is (due to libacl linking) linked against libattr, which is
important!  (This could be problem in libacl - I need to look at it;  but
this is nothing which could be fixed in tar anyway).

Pavel