Re: [PATCH] ls - colorize files with capabilities

[Jim Meyering]

Kamil Dudka <address@hidden> wrote:
> as requested in rhbz #449985 by sectools team it will be good to have
> capability displaying support in ls. This patch has no effect on systems
> without function cap_get_file supported since libcap 2.x. You have to run
> configure with parameter --enable-libcap.

Thanks for working on this.
Sounds worthwhile, though maybe worth
giving it a separate attribute rather than
sharing the one currently used for set-user-ID programs.

Have you considered enabling it by default, when the library is usable?

> From d4fde447cae7d5e40320dd8f7240cd8cb248a127 Mon Sep 17 00:00:00 2001
> From: Kamil Dudka <address@hidden>
> Date: Mon, 14 Jul 2008 13:45:04 +0200

> Subject: [PATCH] Added support for capabilities to ls.

FYI, if the rest were ready, I'd want the log to look like this:

ls: --color now highlights files with capabilities, too
* configure.ac: --enable-libcap configure parameter, check for libcap 2.x 
version
* src/Makefile.am: libcap library linking
* src/ls.c (has_capability): new function for capability detection
(print_color_indicator): colorize file with capability
* NEWS: mentioned the change

Note the s/hasCapability/has_capability/ change.

> ---
>  configure.ac    |   12 ++++++++++++
>  src/Makefile.am |    6 +++---
>  src/ls.c        |   40 +++++++++++++++++++++++++++++++++++++++-
>  NEWS            |    2 ++
>  4 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index ac93e1c..b8567ac 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -44,6 +44,18 @@ gl_EARLY
>  gl_INIT
>  coreutils_MACROS
>
> +dnl Check whether support for libcap 2.x should be built
> +AC_ARG_ENABLE(libcap,
> +  [  --enable-libcap               Enable use of the libcap 2.x library],

It's better to use AC_HELP_STRING:

  AC_ARG_ENABLE([libcap],
    AC_HELP_STRING([--enable-libcap], [enable libcap support])
    AC_HELP_STRING([--disable-libcap], [disable libcap support]),
    ...
    )

> +  [AC_CHECK_LIB([cap], [cap_get_file],
> +    [AC_CHECK_HEADER([sys/capability.h],
> +      [LIB_CAP2="-lcap" AC_DEFINE(HAVE_CAP2, 1, [libcap 2.x availability])],

Re LIB_CAP2,
if you don't embed the library version number in the symbol name,
then we won't have to change it when libcap3 comes along.

> +      [AC_MSG_WARN([header sys/capability.h was not found, support for 
> libcap will not be built])]
> +      )],
> +    [AC_MSG_WARN([libcap 2.x library was not found, support for libcap will 
> not be built])])

Don't mention the 2.x version number here either.

> +    ])
> +AC_SUBST([LIB_CAP2])
> +
>  AC_FUNC_FORK
>
>  optional_bin_progs=
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 65b20a2..e96f98d 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -101,15 +101,15 @@ __LDADD = $(LDADD) $(LIB_EACCESS)
>
>  # for clock_gettime and fdatasync
>  dd_LDADD = $(LDADD) $(LIB_GETHRXTIME) $(LIB_FDATASYNC)
> -dir_LDADD = $(LDADD) $(LIB_CLOCK_GETTIME) $(LIB_SELINUX)
> +dir_LDADD = $(LDADD) $(LIB_CLOCK_GETTIME) $(LIB_SELINUX) $(LIB_CAP2)
>  id_LDADD = $(LDADD) $(LIB_SELINUX)
> -ls_LDADD = $(LDADD) $(LIB_CLOCK_GETTIME) $(LIB_SELINUX)
> +ls_LDADD = $(LDADD) $(LIB_CLOCK_GETTIME) $(LIB_SELINUX) $(LIB_CAP2)
>  mktemp_LDADD = $(LDADD) $(LIB_GETHRXTIME)
>  pr_LDADD = $(LDADD) $(LIB_CLOCK_GETTIME)
>  shred_LDADD = $(LDADD) $(LIB_GETHRXTIME) $(LIB_FDATASYNC)
>  shuf_LDADD = $(LDADD) $(LIB_GETHRXTIME)
>  tac_LDADD = $(LDADD) $(LIB_CLOCK_GETTIME)
> -vdir_LDADD = $(LDADD) $(LIB_CLOCK_GETTIME) $(LIB_SELINUX)
> +vdir_LDADD = $(LDADD) $(LIB_CLOCK_GETTIME) $(LIB_SELINUX) $(LIB_CAP2)
>
>  ## If necessary, add -lm to resolve use of pow in lib/strtod.c.
>  sort_LDADD = $(LDADD) $(POW_LIB) $(LIB_GETHRXTIME)
> diff --git a/src/ls.c b/src/ls.c
> index 4b69f7d..6fc7197 100644
> --- a/src/ls.c
> +++ b/src/ls.c
> @@ -38,6 +38,10 @@
>  #include <config.h>
>  #include <sys/types.h>
>
> +#ifdef HAVE_CAP2
> +# include <sys/capability.h>
> +#endif
> +
>  #if HAVE_TERMIOS_H
>  # include <termios.h>
>  #endif
> @@ -3896,6 +3900,34 @@ print_type_indicator (bool stat_ok, mode_t mode, enum 
> filetype type)
>      DIRED_PUTCHAR (c);
>  }
>
> +#ifdef HAVE_CAP2
> +static bool
> +/* returns true if file has capability (see linux/capability.h) */

Return true if NAME has a capability (see linux/capability.h) */

Use underscores, (i.e., has_capability, has_cap), not mixed case.

> +hasCapability(const char *name)

Always use a space before the opening parenthesis.

> +{
> +  cap_t cap_d;
> +  char *result;
> +  bool hasCap;
> +
> +  cap_d = cap_get_file(name);
> +  if (cap_d == NULL)
> +    return false;
> +
> +  result = cap_to_text(cap_d, NULL);
> +  if (!result) {

curly brace goes on a line by itself.  Use 'gnu indent's style.

> +    cap_free(cap_d);

Move this cap_free call "up".  Then you can remove
the repeated call below.

> +    return false;
> +  }
> +
> +  /* check if human-readable capability string is empty */
> +  hasCap = *result;

I seem to recall that this idiom is slightly better
from a portability standpoint, e.g., when bool is simulated
with an 8-bit type:

    hasCap = !!*result;

> +  cap_free(cap_d);
> +  cap_free(result);
> +  return hasCap;
> +}
> +#endif
> +
>  /* Returns whether any color sequence was printed. */
>  static bool
>  print_color_indicator (const char *name, mode_t mode, int linkok,
> @@ -3919,7 +3951,13 @@ print_color_indicator (const char *name, mode_t mode, 
> int linkok,
>        if (S_ISREG (mode))
>       {
>         type = C_FILE;
> -       if ((mode & S_ISUID) != 0)
> +       if (
> +        ((mode & S_ISUID) != 0)
> +#ifdef HAVE_CAP2

Remove these in-function #ifdefs.
Instead, define has_capability to return false when the library
is not available.

> +/* highlights file with capability (see linux/capability.h) */
> +        || hasCapability(name)
> +#endif
> +        )
>           type = C_SETUID;
>         else if ((mode & S_ISGID) != 0)
>           type = C_SETGID;
> diff --git a/NEWS b/NEWS
> index d6ed89e..16b721e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -27,6 +27,8 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>    represents the maximum number of inputs that will be merged at once.
>    When processing more than NMERGE inputs, sort uses temporary files.
>
> +  ls now colorizes files with capabilities if libcap is available
> +
>  ** Bug fixes
>
>    chcon --verbose now prints a newline after each message