findutils-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Findutils-patches] Rebased extended attribute (-xattr) and capabilt


From: Bernhard Voelker
Subject: Re: [Findutils-patches] Rebased extended attribute (-xattr) and capabilty (-cap) patch for find
Date: Thu, 19 Jul 2018 01:04:51 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.0

On 07/17/2018 08:44 AM, Pavel Raiskup wrote:
> Hi Morgan, thanks for the update!  (in-line patch would be better)

Indeed, ideally created with "git format-patch -1".

> For maintainers, since Morgan is Red Hat employee no copyright paperwork
> is needed.

great, thanks.

> On Tuesday, July 17, 2018 2:05:15 AM CEST Morgan Weetman wrote:
>> diff --git a/configure.ac b/configure.ac
>> index 2acf54c7..ab286b6c 100644
> 
> You should write a GNU formatted commit message here, like:
> 
>   topic: short comment
> 
>   Long description.
> 
>   * file_changed (method_changed_or_added): Description.
> 
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -55,6 +55,26 @@ AC_SUBST(AUXDIR,$ac_aux_dir)
>>  dnl check for --with-fts
>>  FIND_WITH_FTS
>>  
>> +dnl Disable extended attribute support
>> +AC_ARG_WITH([extended-attributes],
>> +    AS_HELP_STRING([--without-extended-attributes], [Disable extended 
>> attributes support]))
>> +
>> +AS_IF([test "x$with_extended_attributes" != "xno"], [
>> +  dnl Search for libattr
>> +  AC_SEARCH_LIBS([listxattr], [attr], [AC_DEFINE([HAVE_XATTR], [],
>> +    [libattr is present])], [])
>> +])
> 
> From what I remember, Darwin defines this method, but not llistxattr which
> is also used by the code below.  I'd say you should check for both
> methods.  More discussion in GNU tar list:
> https://www.mail-archive.com/address@hidden/msg04586.html
> 
>> +dnl Disable capabilities
>> +AC_ARG_WITH([capabilities],
>> +    AS_HELP_STRING([--without-capabilities], [Disable capabilities 
>> support]))
>> +
>> +AS_IF([test "x$with_capabilities" != "xno"], [
>> +  dnl Search for libcap
>> +  AC_SEARCH_LIBS([cap_get_file], [cap], [AC_DEFINE([HAVE_CAPABILITIES], [],
>> +    [libcap is present])], [])
>> +])
>> +
>>  AC_ARG_ENABLE(leaf-optimisation,
>>      AS_HELP_STRING(--enable-leaf-optimisation,Enable an optimisation which 
>> saves lstat calls to identify subdirectories on filesystems having 
>> traditional Unix semantics),
>>      [ac_cv_leaf_optimisation=$enableval],[ac_cv_leaf_optimisation=yes])
>> diff --git a/find/defs.h b/find/defs.h
>> index 63f3b2a3..bde58c5d 100644
>> --- a/find/defs.h
>> +++ b/find/defs.h
>> @@ -417,6 +417,7 @@ PREDICATEFUNCTION pred_amin;
>>  PREDICATEFUNCTION pred_and;
>>  PREDICATEFUNCTION pred_anewer;
>>  PREDICATEFUNCTION pred_atime;
>> +PREDICATEFUNCTION pred_cap;
>>  PREDICATEFUNCTION pred_closeparen;
>>  PREDICATEFUNCTION pred_cmin;
>>  PREDICATEFUNCTION pred_cnewer;
>> @@ -435,10 +436,12 @@ PREDICATEFUNCTION pred_fprintf;
>>  PREDICATEFUNCTION pred_fstype;
>>  PREDICATEFUNCTION pred_gid;
>>  PREDICATEFUNCTION pred_group;
>> +PREDICATEFUNCTION pred_icap;
>>  PREDICATEFUNCTION pred_ilname;
>>  PREDICATEFUNCTION pred_iname;
>>  PREDICATEFUNCTION pred_inum;
>>  PREDICATEFUNCTION pred_ipath;
>> +PREDICATEFUNCTION pred_ixattr;
>>  PREDICATEFUNCTION pred_links;
>>  PREDICATEFUNCTION pred_lname;
>>  PREDICATEFUNCTION pred_ls;
>> @@ -469,6 +472,7 @@ PREDICATEFUNCTION pred_uid;
>>  PREDICATEFUNCTION pred_used;
>>  PREDICATEFUNCTION pred_user;
>>  PREDICATEFUNCTION pred_writable;
>> +PREDICATEFUNCTION pred_xattr;
>>  PREDICATEFUNCTION pred_xtype;
>>  PREDICATEFUNCTION pred_context;
>>  
>> diff --git a/find/find.1 b/find/find.1
>> index 0d44c37c..2e48983b 100644
>> --- a/find/find.1
>> +++ b/find/find.1
>> @@ -609,6 +609,13 @@ a file has to have been accessed at least
>>  .I two
>>  days ago.
>>  
>> +.IP "\-cap \fIpattern\fR"
>> +Match file capabilities against the regular expression
>> +\fIpattern\fR. For example to search for files that have
>> +CAP_SETUID you could use '.*setuid.*'. This option
>> +also takes advantage of
>> +.B \-regextype
>> +
>>  .IP "\-cmin \fIn\fR"
>>  File's status was last changed \fIn\fR minutes ago.
>>  
>> @@ -671,6 +678,11 @@ File's numeric group ID is
>>  .IP "\-group \fIgname\fR"
>>  File belongs to group \fIgname\fR (numeric group ID allowed).
>>  
>> +.IP "\-icap \fIpattern\fR"
>> +Like
>> +.BR \-cap ,
>> +but the match is case insensitive.
>> +
>>  .IP "\-ilname \fIpattern\fR"
>>  Like
>>  .BR \-lname ,
>> @@ -712,6 +724,11 @@ but the match is case insensitive.
>>  See \-ipath.  This alternative is less portable than
>>  .BR \-ipath .
>>  
>> +.IP "\-ixattr \fIpattern\fR"
>> +Like
>> +.BR \-xattr ,
>> +but the match is case insensitive.
>> +
>>  .IP "\-links \fIn\fR"
>>  File has \fIn\fR hard links.
>>  
>> @@ -1036,6 +1053,13 @@ mapping (or root-squashing), since many systems 
>> implement
>>  in the client's kernel and so cannot make use of the UID mapping
>>  information held on the server.
>>  
>> +.IP "\-xattr \fIpattern\fR"
>> +Match extended attributes against the regular expression
>> +\fIpattern\fR. For example to search for files that have
>> +capabilities set you could use '.*capa.*'. This option
>> +also takes advantage of
>> +.B \-regextype
>> +
>>  .IP "\-xtype \fIc\fR"
>>  The same as
>>  .B \-type
>> diff --git a/find/parser.c b/find/parser.c
>> index d6621506..b4d6e594 100644
>> --- a/find/parser.c
>> +++ b/find/parser.c
>> @@ -79,6 +79,9 @@ static bool parse_accesscheck   (const struct 
>> parser_table*, char *argv[], int *
>>  static bool parse_amin          (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_and           (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_anewer        (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> +#ifdef HAVE_CAPABILITIES
> 
> I don't think the #ifdefs are needed here, see below..
> 
>> +static bool parse_cap           (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> +#endif
>>  static bool parse_cmin          (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_cnewer        (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_comma         (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> @@ -98,12 +101,18 @@ static bool parse_fprint0       (const struct 
>> parser_table*, char *argv[], int *
>>  static bool parse_fstype        (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_gid           (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_group         (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> +#ifdef HAVE_CAPABILITIES
>> +static bool parse_icap          (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> +#endif
>>  static bool parse_ilname        (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_iname         (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_inum          (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_ipath         (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_iregex        (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_iwholename    (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> +#ifdef HAVE_XATTR
>> +static bool parse_ixattr        (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> +#endif
>>  static bool parse_links         (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_lname         (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_ls            (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> @@ -141,6 +150,9 @@ static bool parse_xdev          (const struct 
>> parser_table*, char *argv[], int *
>>  static bool parse_ignore_race   (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_noignore_race (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_warn          (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> +#ifdef HAVE_XATTR
>> +static bool parse_xattr         (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> +#endif
>>  static bool parse_xtype         (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_quit          (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>>  static bool parse_context       (const struct parser_table*, char *argv[], 
>> int *arg_ptr);
>> @@ -154,6 +166,11 @@ static bool parse_version       (const struct 
>> parser_table*, char *argv[], int *
>>    _GL_ATTRIBUTE_NORETURN;
>>  
>>  
>> +#ifdef HAVE_CAPABILITIES
>> +static bool insert_cap (char *argv[], int *arg_ptr,
>> +        const struct parser_table *entry,
>> +        int regex_options);
>> +#endif
>>  static bool insert_type (char **argv, int *arg_ptr,
>>                       const struct parser_table *entry,
>>                       PRED_FUNC which_pred);
>> @@ -164,6 +181,11 @@ static bool insert_exec_ok (const char *action,
>>                          const struct parser_table *entry,
>>                          char *argv[],
>>                          int *arg_ptr);
>> +#ifdef HAVE_XATTR
>> +static bool insert_xattr (char *argv[], int *arg_ptr,
>> +        const struct parser_table *entry,
>> +        int regex_options);
>> +#endif
>>  static bool get_comp_type (const char **str,
>>                         enum comparison_type *comp_type);
>>  static bool get_relative_timestamp (const char *str,
>> @@ -229,6 +251,9 @@ static struct parser_table const parse_table[] =
>>    PARSE_PUNCTUATION("and",                   and),          /* GNU */
>>    PARSE_TEST       ("anewer",                anewer),            /* GNU */
>>    {ARG_TEST,       "atime",                  parse_time, pred_atime}, /* 
>> POSIX */
>> +  #ifdef HAVE_CAPABILITIES
>> +  PARSE_TEST       ("cap",                   cap),          /* GNU */
>> +#endif
>>    PARSE_TEST       ("cmin",                  cmin),      /* GNU */
>>    PARSE_TEST       ("cnewer",                cnewer),            /* GNU */
>>    {ARG_TEST,       "ctime",                  parse_time, pred_ctime}, /* 
>> POSIX */
>> @@ -249,6 +274,9 @@ static struct parser_table const parse_table[] =
>>    PARSE_TEST       ("fstype",                fstype),  /* GNU, Unix */
>>    PARSE_TEST       ("gid",                   gid),       /* GNU */
>>    PARSE_TEST       ("group",                 group), /* POSIX */
>> +#ifdef HAVE_CAPABILITIES
>> +  PARSE_TEST_NP    ("icap",                  icap),         /* GNU */
>> +#endif
>>    PARSE_OPTION     ("ignore_readdir_race",   ignore_race),   /* GNU */
>>    PARSE_TEST       ("ilname",                ilname),            /* GNU */
>>    PARSE_TEST       ("iname",                 iname),             /* GNU */
>> @@ -256,6 +284,9 @@ static struct parser_table const parse_table[] =
>>    PARSE_TEST       ("ipath",                 ipath), /* GNU, deprecated in 
>> favour of iwholename */
>>    PARSE_TEST_NP    ("iregex",                iregex),            /* GNU */
>>    PARSE_TEST_NP    ("iwholename",            iwholename),    /* GNU */
>> +#ifdef HAVE_XATTR
>> +  PARSE_TEST_NP    ("ixattr",                ixattr),       /* GNU */
>> +#endif
>>    PARSE_TEST       ("links",                 links), /* POSIX */
>>    PARSE_TEST       ("lname",                 lname),             /* GNU */
>>    PARSE_ACTION     ("ls",                    ls),      /* GNU, Unix */
>> @@ -301,6 +332,9 @@ static struct parser_table const parse_table[] =
>>    PARSE_TEST       ("user",                  user), /* POSIX */
>>    PARSE_TEST_NP    ("wholename",             wholename), /* GNU, replaced 
>> -path, but now -path is standardized since POSIX 2008 */
>>    {ARG_TEST,       "writable",               parse_accesscheck, 
>> pred_writable}, /* GNU, 4.3.0+ */
>> +#ifdef HAVE_XATTR
>> +  PARSE_TEST       ("xattr",                 xattr),        /* GNU */
>> +#endif
>>    PARSE_OPTION     ("xdev",                  xdev), /* POSIX */
>>    PARSE_TEST       ("xtype",                 xtype),             /* GNU */
>>  #ifdef UNIMPLEMENTED_UNIX
>> @@ -797,6 +831,14 @@ parse_anewer (const struct parser_table* entry, char 
>> **argv, int *arg_ptr)
>>    return false;
>>  }
>>  
>> +#ifdef HAVE_CAPABILITIES
>> +static bool
>> +parse_cap (const struct parser_table* entry, char **argv, int *arg_ptr)
>> +{
>> +  return insert_cap (argv, arg_ptr, entry, options.regex_options);
>> +}
>> +#endif
>> +
>>  bool
>>  parse_closeparen (const struct parser_table* entry, char **argv, int 
>> *arg_ptr)
>>  {
>> @@ -1212,6 +1254,14 @@ estimate_pattern_match_rate (const char *pattern, int 
>> is_regex)
>>      }
>>  }
>>  
>> +#ifdef HAVE_CAPABILITIES
>> +static bool
>> +parse_icap (const struct parser_table* entry, char **argv, int *arg_ptr)
>> +{
>> +  return insert_cap (argv, arg_ptr, entry, RE_ICASE|options.regex_options);
>> +}
>> +#endif
>> +
>>  static bool
>>  parse_ilname (const struct parser_table* entry, char **argv, int *arg_ptr)
>>  {
>> @@ -1324,6 +1374,14 @@ parse_iregex (const struct parser_table* entry, char 
>> **argv, int *arg_ptr)
>>    return insert_regex (argv, arg_ptr, entry, 
>> RE_ICASE|options.regex_options);
>>  }
>>  
>> +#ifdef HAVE_XATTR
>> +static bool
>> +parse_ixattr (const struct parser_table* entry, char **argv, int *arg_ptr)
>> +{
>> +  return insert_xattr (argv, arg_ptr, entry, 
>> RE_ICASE|options.regex_options);
>> +}
>> +#endif
>> +
>>  static bool
>>  parse_links (const struct parser_table* entry, char **argv, int *arg_ptr)
>>  {
>> @@ -2627,6 +2685,46 @@ parse_warn (const struct parser_table* entry, char 
>> **argv, int *arg_ptr)
>>    return parse_noop (entry, argv, arg_ptr);
>>  }
>>  
>> +#ifdef HAVE_XATTR
>> +static bool
>> +parse_xattr (const struct parser_table* entry, char **argv, int *arg_ptr)
>> +{
>> +  return insert_xattr (argv, arg_ptr, entry, options.regex_options);
>> +}
>> +
>> +static bool
>> +insert_xattr (char **argv,
>> +              int *arg_ptr,
>> +              const struct parser_table *entry,
>> +              int regex_options)
>> +{
>> +  const char *rx;
>> +  if (collect_arg (argv, arg_ptr, &rx))
>> +    {
>> +      struct re_pattern_buffer *re;
>> +      const char *error_message;
>> +      struct predicate *our_pred = insert_primary_withpred (entry, 
>> pred_xattr, rx);
>> +      our_pred->need_stat = our_pred->need_type = false;
>> +      re = xmalloc (sizeof (struct re_pattern_buffer));
>> +      our_pred->args.regex = re;
>> +      re->allocated = 100;
>> +      re->buffer = xmalloc (re->allocated);
>> +      re->fastmap = NULL;
>> +
>> +      re_set_syntax (regex_options);
>> +      re->syntax = regex_options;
>> +      re->translate = NULL;
>> +
>> +      error_message = re_compile_pattern (rx, strlen (rx), re);
>> +      if (error_message)
>> +        error (EXIT_FAILURE, 0, "%s", error_message);

Use die() with a translatable _("...") error message, please.

>> +      our_pred->est_success_rate = estimate_pattern_match_rate (rx, 1);
>> +      return true;
>> +    }
>> +  return false;
>> +}
>> +#endif
>> +
>>  static bool
>>  parse_xtype (const struct parser_table* entry, char **argv, int *arg_ptr)
>>  {
>> @@ -2877,6 +2975,40 @@ check_path_safety (const char *action)
>>      } while (splitstring (path, path_separators, false, &pos, &len));
>>  }
>>  
>> +#ifdef HAVE_CAPABILITIES
>> +static bool
>> +insert_cap (char **argv,
>> +              int *arg_ptr,
>> +              const struct parser_table *entry,
>> +              int regex_options)
>> +{
>> +  const char *rx;
>> +  if (collect_arg (argv, arg_ptr, &rx))
>> +    {
>> +      struct re_pattern_buffer *re;
>> +      const char *error_message;
>> +      struct predicate *our_pred = insert_primary_withpred (entry, 
>> pred_cap, rx);
>> +      our_pred->need_stat = true;
>> +      our_pred->need_type = false;
>> +      re = xmalloc (sizeof (struct re_pattern_buffer));
>> +      our_pred->args.regex = re;
>> +      re->allocated = 100;
>> +      re->buffer = xmalloc (re->allocated);
>> +      re->fastmap = NULL;
>> +
>> +      re_set_syntax (regex_options);
>> +      re->syntax = regex_options;
>> +      re->translate = NULL;
>> +
>> +      error_message = re_compile_pattern (rx, strlen (rx), re);
>> +      if (error_message)
>> +        error (EXIT_FAILURE, 0, "%s", error_message);

Use die() with a translatable _("...") error message, please.

>> +      our_pred->est_success_rate = estimate_pattern_match_rate (rx, 1);
>> +      return true;
>> +    }
>> +  return false;
>> +}
>> +#endif

Besides 'insert_regex', we now have 3 almost identical functions to
store a pre-compiled regular expression in the predicate tree.
It probably makes sense to factor it out, hmm?

>>  /* handles both exec and ok predicate */
>>  static bool
>> diff --git a/find/pred.c b/find/pred.c
>> index 2014b5ab..08ad1b8a 100644
>> --- a/find/pred.c
>> +++ b/find/pred.c
>> @@ -34,6 +34,14 @@
>>  #include <sys/wait.h>
>>  #include <unistd.h> /* for unlinkat() */
>>  
>> +#ifdef HAVE_XATTR
>> +#include <attr/xattr.h>
> 
> Nowadays you should probably use <sys/xattr.h>, and maybe fallback to
> <attr/xattr.h> (that header is provided by glibc-headers).
> 
>> +#endif
>> +
>> +#ifdef HAVE_CAPABILITIES
>> +#include <sys/capability.h> /* for pred_cap() */
>> +#endif
>> +
>>  /* gnulib headers. */
>>  #include "areadlink.h"
>>  #include "dirname.h"
>> @@ -74,6 +82,9 @@ struct pred_assoc pred_table[] =
>>    {pred_and, "and     "},
>>    {pred_anewer, "anewer  "},
>>    {pred_atime, "atime   "},
>> +#ifdef HAVE_CAPABILITIES
>> +  {pred_cap, "cap    "},
>> +#endif
>>    {pred_closeparen, ")       "},
>>    {pred_cmin, "cmin    "},
>>    {pred_cnewer, "cnewer  "},
>> @@ -126,6 +137,9 @@ struct pred_assoc pred_table[] =
>>    {pred_used, "used    "},
>>    {pred_user, "user    "},
>>    {pred_writable, "writable "},
>> +#ifdef HAVE_XATTR
>> +  {pred_xattr, "xattr   "},
>> +#endif
>>    {pred_xtype, "xtype   "},
>>    {pred_context, "context"},
>>    {0, "none    "}
>> @@ -242,6 +256,34 @@ pred_atime (const char *pathname, struct stat 
>> *stat_buf, struct predicate *pred_
>>    return pred_timewindow (get_stat_atime(stat_buf), pred_ptr, DAYSECS);
>>  }
>>  
>> +#ifdef HAVE_CAPABILITIES
>> +bool
>> +pred_cap (const char *pathname, struct stat *stat_buf, struct predicate 
>> *pred_ptr)
>> +{
>> +  (void) pathname;

why that?

>> +  char *cap_str;
>> +  cap_t caps;
>> +  bool ret = false;
> 
> Looking how pred_context() has been implemented, I think that in
> pret_cap() this pattern ...
> 
>   #ifndef HAVE_CAPABILITIES
>     errno = ENOTSUP;
>     error (0, errno, _("can't get capabilities, not implemented: %s"),
>            safely_quote_err_filename (0, pathname));
>     return (ret);
>   #else
>     ... the code calling cap_* stuff goes here ...
>   #endif
> 
> .. should be used.  This way you can avoid a lot of the #ifdef hell since
> everything can be complied in.  Findutils maintainers should review this,
> though.

+1
That would also mean that the new options -xattr and -cap are visible
also in executables which do not support the features.  That in turn
makes the documentation (--help, find.1, find.texi) more consistent.

>> +  // get capabilities
> 
> I think that only /* comments */ are allowed.
> 
>> +  caps = cap_get_file(pathname);

please check for (caps == NULL) here as well.  It's not described explicitly
in the man page what would happen when one calls cap_to_text(NULL, NULL),
so it's safer to check before ...  with an error diagnostic.

>> +  cap_str = cap_to_text(caps, NULL);
>> +  if ( cap_str == NULL ) {

find should issue an error diagnostic here, too.

> Style note, that should be `if (cap_str == NULL)`.

or `if (NULL == cap_str)` ;-)

>> +    cap_free(caps);
>> +    return(ret);
>> +  }
>> +
>> +  // perform regex match
>> +  if (regexec(pred_ptr->args.regex, cap_str, (size_t) 0, NULL, 0) == 0)
>> +    ret = true;
>> +
>> +  // clean up
>> +  cap_free(caps);
>> +  cap_free(cap_str);
>> +  return(ret);
>> +}
>> +#endif
>> +
>>  bool
>>  pred_closeparen (const char *pathname, struct stat *stat_buf, struct 
>> predicate *pred_ptr)
>>  {
>> @@ -1184,6 +1226,61 @@ pred_user (const char *pathname, struct stat 
>> *stat_buf, struct predicate *pred_p
>>      return (false);
>>  }
>>  
>> +#ifdef HAVE_XATTR
>> +bool
>> +pred_xattr (const char *pathname, struct stat *stat_buf, struct predicate 
>> *pred_ptr)
>> +{
>> +  (void) pathname;
>> +  char empty[0];

no need for 'empty' .. just pass NULL.

>> +  char *list;
>> +  char *substrings;
>> +  ssize_t list_size;
>> +  int i, j;
>> +  bool ret = false;

IMO too many variables here.
Would you please give them a better scope?

>> +
>> +  // get size of xattr list for the given path by passing an empty list
>> +  if (options.symlink_handling == SYMLINK_NEVER_DEREF) {

... else: what about SYMLINKS_DEREF_ARGSONLY?

>> +    list_size = llistxattr(pathname, empty, 0);
> 
> Function calls should look like `function<space>(param1, ...)`.
> 
>> +  } else {
> 
> The 'else' keyword is always on it's own line.
> 
> Thanks again!
> Pavel
> 
>> +    list_size = listxattr(pathname, empty, 0);
>> +  }

Please check `if (list_size < 0)`.

BTW: ENOTSUP could be used to skip further checks on the same
file system.

>> +  // allocate just enough memory to hold all xattrs
>> +  list = malloc(list_size);

s/malloc/xmalloc/

>> +
>> +  // used to hold individual attributes (substrings)
>> +  // allocate same size as list just in case there's only one xattr
>> +  substrings = malloc(list_size);

I don't think you need substrings.
Please have a look at the example code in 'man listxattr'.

>> +
>> +  // retrieve the list of xattrs
>> +  if (options.symlink_handling == SYMLINK_NEVER_DEREF) {
>> +    llistxattr(pathname, list, list_size);
>> +  } else {
>> +    listxattr(pathname, list, list_size);

Man says:
  If  size  is  specified as zero, these calls return the current size
  of the list of extended attribute names (and leave list unchanged).
  This can be used to determine the size of the buffer that should be
  supplied in a subsequent call.  (But, bear in mind that there is a
  possibility that the set of extended attributes may change between
  the two calls, so that it is still necessary to check the return
  status from the second call.)

In the ERANGE case, find may need to xrealloc and retry.

Other errno values could happen if e.g. the file got removed since
the call above ... yes, file systems do get modified once in a while.
;-)

>> +  }

again: what about the SYMLINK_DEREF_ARGSONLY?

>> +
>> +  // break list into asciiz strings
>> +  for (i = 0, j = 0; i < list_size; i++) {

As said above: the 2-variable loop isn't necessary.

>> +    substrings[j] = list[i];
>> +    if (list[i] == 0) {
>> +      // perform regex match against substring
>> +      j = 0;
>> +      if (regexec(pred_ptr->args.regex, substrings, (size_t) 0, NULL, 0) == 
>> 0) {
>> +        ret = true;

break; ?

>> +      }
>> +      continue;
>> +    }
>> +    j++;
>> +  }
>> +
>> +  // clean up
>> +  free(list);
>> +  free(substrings);
>> +  return(ret);
>> +
>> +}
>> +#endif
>> +
>>  bool
>>  pred_xtype (const char *pathname, struct stat *stat_buf, struct predicate 
>> *pred_ptr)
>>  {
>> diff --git a/find/testsuite/excuses.txt b/find/testsuite/excuses.txt
>> index ac4515a1..d4aee6d4 100644
>> --- a/find/testsuite/excuses.txt
>> +++ b/find/testsuite/excuses.txt
>> @@ -22,6 +22,7 @@
>>  
>>  ## Things that are hard to test because they rely on features of
>>  ## the environment
>> +"cap",                   cap),      /* GNU */
>>  "gid",                   gid),           /* GNU */
>>  "uid",                   uid),           /* GNU */
>>  "user",                  user),
>> @@ -34,6 +35,7 @@
>>  "ignore_readdir_race",   ignore_race),   /* GNU */
>>  "noignore_readdir_race", noignore_race), /* GNU */
>>  "noleaf",                noleaf),        /* GNU */
>> +"xattr",                 xattr),            /* GNU */
>>  
>>  ## Things that might be testable but which I have not yet thought
>>  ## about enough.
>> diff --git a/find/tree.c b/find/tree.c
>> index 5be88f2e..d5938a0c 100644
>> --- a/find/tree.c
>> +++ b/find/tree.c
>> @@ -926,6 +926,9 @@ static struct pred_cost_lookup costlookup[] =
>>      { pred_and       ,  NeedsNothing,        },
>>      { pred_anewer    ,  NeedsStatInfo,       },
>>      { pred_atime     ,  NeedsStatInfo,       },
>> +#ifdef HAVE_CAPABILITIES
>> +    { pred_cap       ,  NeedsNothing,        },
>> +#endif
>>      { pred_closeparen,  NeedsNothing         },
>>      { pred_cmin      ,  NeedsStatInfo,       },
>>      { pred_cnewer    ,  NeedsStatInfo,       },
>> @@ -980,6 +983,9 @@ static struct pred_cost_lookup costlookup[] =
>>      { pred_used      ,  NeedsStatInfo        },
>>      { pred_user      ,  NeedsStatInfo        },
>>      { pred_writable  ,  NeedsAccessInfo      },
>> +#ifdef HAVE_XATTR
>> +    { pred_xattr     ,  NeedsNothing         },
>> +#endif
>>      { pred_xtype     ,  NeedsType            } /* roughly correct unless 
>> most files are symlinks */
>>    };
>>  static int pred_table_sorted = 0;
>> diff --git a/find/util.c b/find/util.c
>> index fa338074..0b2e8646 100644
>> --- a/find/util.c
>> +++ b/find/util.c
>> @@ -181,15 +181,16 @@ normal options (always true, specified before other 
>> expressions):\n\
>>        -depth --help -maxdepth LEVELS -mindepth LEVELS -mount -noleaf\n\
>>        --version -xdev -ignore_readdir_race -noignore_readdir_race\n"));
>>    HTL (_("\
>> -tests (N can be +N or -N or N): -amin N -anewer FILE -atime N -cmin N\n\
>> -      -cnewer FILE -ctime N -empty -false -fstype TYPE -gid N -group NAME\n\
>> -      -ilname PATTERN -iname PATTERN -inum N -iwholename PATTERN -iregex 
>> PATTERN\n\
>> -      -links N -lname PATTERN -mmin N -mtime N -name PATTERN -newer FILE"));
>> +tests (N can be +N or -N or N): -amin N -anewer FILE -atime N -cap 
>> PATTERN\n\
>> +      -cmin N -cnewer FILE -ctime N -empty -false -fstype TYPE -gid N 
>> -group NAME\n\
>> +      -icap PATTERN -ilname PATTERN -iname PATTERN -inum N -iwholename 
>> PATTERN\n\
>> +      -iregex PATTERN -ixattr PATTERN -links N -lname PATTERN -mmin N 
>> -mtime N\n\
>> +      -name PATTERN -newer FILE"));
>>    HTL (_("\n\
>>        -nouser -nogroup -path PATTERN -perm [-/]MODE -regex PATTERN\n\
>>        -readable -writable -executable\n\
>>        -wholename PATTERN -size N[bcwkMG] -true -type [bcdpflsD] -uid N\n\
>> -      -used N -user NAME -xtype [bcdpfls]"));
>> +      -used N -user NAME -xattr PATTERN -xtype [bcdpfls]"));
>>    HTL (_("\
>>        -context CONTEXT\n"));
>>    HTL (_("\n\

Many thanks for all the work so far.

A few thoughts:

a) what about the need to search for files with any caps/xattrs?
"find -xattr '.*'" is probably fine yet not obvious to the user,
so this warrants to be added to the documentation.

Still, for this case we could shortcut the whole regex stuff.
Maybe the special case "find -xattr ''" could be used?
(Luckily, the regexec implementation seems to return a positive match,
because you didn't treat that case specially.)

b) what about the need to search for files /not/ having caps/xattrs?
Does "find '!' -xattr '.*'" suffice? Should be documented as well.

c) a shell-based test isn't too hard to write - it just has to be skipped
when either of setfattr/getfattr fail.  Likewise probably for getcap/setcap.

d) documentation: GNU prefers the Texinfo manual to be the primary
documentation material.  Well, findutils still has both, i.e., a full-blown
man page and a Texinfo manual, so the new options should go into both.

e) what about an option to search for files with certain xattr values?
E.g.:

  $ setfattr -n user.fred -v chocolate /tmp/foo

  $ find /tmp -xattrvalue '.*late'
or
  $ find /tmp -xattrnamevalue 'user\..*' '.*late'

Does anyone consider this useful?
Well, this would go definitely into another patch.

Thanks & have a nice day,
Berny







reply via email to

[Prev in Thread] Current Thread [Next in Thread]