[PATCH v3] allow disabling ACL

[Peter Wu]

Hi David,

The overall patch looks fine to me, but it got mangled by your mail
client. Some minor nitpicks/comment below.

On Friday 04 April 2014 15:47:56 David Heidelberger wrote:
>  From 310d26a9ad845c0a320692c873562b37c94cd102 Mon Sep 17 00:00:00 2001
>  From: David Heidelberger <david.heidelberger at ixit.cz>
> Date: Thu, 13 Mar 2014 21:28:42 +0100
> Subject: [PATCH v3] allow disabling ACL
> 
> This patch provide option to build and run udisks without ACL.
> v2: as replacement of ACL is used chown call
> v3: do not change uid, change gid
> ---
>   configure.ac                | 38 ++++++++++++++++++++++++++------------
>   src/udiskslinuxfilesystem.c | 14 ++++++++++++--
>   2 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 3a39b5a..e656abf 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -174,18 +174,31 @@ if test "x$with_systemdsystemunitdir" != "xno"; 
> then
>   fi
>   AM_CONDITIONAL(HAVE_SYSTEMD, [test -n "$systemdsystemunitdir"])
> 
> -# libacl
> -AC_CHECK_HEADERS(
> -        [sys/acl.h acl/libacl.h],
> -        [ACL_CFLAGS=""],
> -        AC_MSG_ERROR([*** ACL headers not found.]))
> -AC_CHECK_LIB(
> -        [acl],
> -        [acl_get_file],
> -        [ACL_LIBS="-lacl"],
> -        AC_MSG_ERROR([*** libacl not found.]))
> -AC_SUBST(ACL_CFLAGS)
> -AC_SUBST(ACL_LIBS)
> +have_acl=no
> +AC_ARG_ENABLE(acl, AS_HELP_STRING([--disable-acl], [disable acl 
> support]))
> +if test "x$enable_acl" != "xno"; then
> +  AC_CHECK_HEADERS(
> +          [sys/acl.h acl/libacl.h],
> +          [
> +            AC_CHECK_LIB(
> +                [acl],
> +                [acl_get_file],
> +                [AC_DEFINE(HAVE_ACL, 1, [Define if libacl is 
> available]) have_acl=yes],
> +                have_acl=no)
> +          ],
> +          have_acl=no)
> +  if test "x$have_acl" = "xyes"; then
> +    ACL_CFLAGS=""
> +    ACL_LIBS="-lacl"
> +  fi
> +  AC_SUBST(ACL_CFLAGS)
> +  AC_SUBST(ACL_LIBS)
> +  if test "x$have_acl" = xno -a "x$enable_acl" = xyes; then
> +    AC_MSG_ERROR([acl support requested but libraries not found])
> +  fi
> +fi
> +AM_CONDITIONAL(HAVE_ACL, [test "$have_acl" = "yes"])
> +
> 
>   # Internationalization
>   #
> @@ -232,6 +245,7 @@ echo "
>           udevdir:                    ${udevdir}
>           systemdsystemunitdir:       ${systemdsystemunitdir}
>           using libsystemd-login:     ${have_libsystemd_login}
> +	acl support:                ${have_acl}

Shouldn't this be indented with spaces?

> 
>           compiler:                   ${CC}
>           cflags:                     ${CFLAGS}
> diff --git a/src/udiskslinuxfilesystem.c b/src/udiskslinuxfilesystem.c
> index f243046..0b9bdbf 100644
> --- a/src/udiskslinuxfilesystem.c
> +++ b/src/udiskslinuxfilesystem.c
> @@ -29,7 +29,9 @@
>   #include <stdio.h>
>   #include <mntent.h>
>   #include <sys/types.h>
> +#ifdef HAVE_ACL
>   #include <sys/acl.h>
> +#endif
>   #include <errno.h>
> 
>   #include <glib/gstdio.h>
> @@ -795,7 +797,7 @@ ensure_utf8 (const gchar *s)
>   }
> 
>   /* 
> ---------------------------------------------------------------------------------------------------- 
> */
> -

Can this new line be kept?

> +#ifdef HAVE_ACL
>   static gboolean
>   add_acl (const gchar  *path,
>            uid_t         uid,
> @@ -831,7 +833,7 @@ add_acl (const gchar  *path,
>       acl_free (acl);
>     return ret;
>   }
> -
> +#endif

Can there be a newline after this?

>   /*
>    * calculate_mount_point: <internal>
>    * @dameon: A #UDisksDaemon.
> @@ -911,7 +913,11 @@ calculate_mount_point (UDisksDaemon              
> *daemon,
>                   }
>               }
>             /* Then create the per-user /run/media/$USER */
> +#ifdef HAVE_ACL
>             if (g_mkdir (mount_dir, 0700) != 0)
> +#else
> +          if (g_mkdir (mount_dir, 0750) != 0)

It does not look aligned, is there a tab/space mismatch here?

> +#endif
>               {
>                 g_set_error (error,
>                              UDISKS_ERROR,
> @@ -921,7 +927,11 @@ calculate_mount_point (UDisksDaemon              
> *daemon,
>                 goto out;
>               }
>             /* Finally, add the read+execute ACL for $USER */
> +#ifdef HAVE_ACL
>             if (!add_acl (mount_dir, uid, error))
> +#else
> +          if (chown (mount_dir, -1, gid) == -1)

add_acl seems to report an error if something fails, wouldn't that also
be useful here?

> +#endif
>               {
>                 if (rmdir (mount_dir) != 0)
>                   udisks_warning ("Error calling rmdir() on %s: %m", 
> mount_dir);
> 

Kind regards,
Peter