Bug 48842 - Use libacl instead of spawning setfacl(1)
Use libacl instead of spawning setfacl(1)
Status: RESOLVED FIXED
Product: udisks
Classification: Unclassified
Component: general
unspecified
All Linux (All)
: medium major
Assigned To: David Zeuthen (not reading bugmail)
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-17 12:36 UTC by Samuli Suominen
Modified: 2012-04-20 10:09 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Samuli Suominen 2012-04-17 12:36:48 UTC
src/udiskslinuxfilesystem.c in git and udisks-1.94.0 has this code:

          /* Then set the ACL such that only $USER can actually access it */
          escaped_user_name = udisks_daemon_util_escape (user_name);;
          s = g_strdup_printf ("setfacl -m \"u:%s:rx\" \"%s\"",
                               escaped_user_name,
                               mount_dir);

but configure.ac is missing a check for the command. it should propably fail if the command is not found because as per this bug report:

http://bugs.gentoo.org/412377

it's actually preventing from volumes getting mounted. 

the user in the downstream bug is also asking for this depend to be made optional. i'm just going to quote him here:

"After upgrading to gnome-base/gvfs-1.12.0 volumes fail to mount e. g. in nautilus with this error message:

Unable to mount <volume>
Failed to execute child process "setfacl" (No such file or directory)

The obvious cause of this is that I do not have sys-apps/acl installed as ACLs are not compiled into the filesystems in my kernel.

gvfs should not use ACL utilities if not present or at least support USE="-acl".

Downgrading to gnome-base/gvfs-1.10.1 solves this (for now).

Reproducible: Always

Steps to Reproduce:
1. emerge =gnome-base/gvfs-1.12.0
2. Open nautilus
3. Click a mountable volume that is not mounted yet
Actual Results:  
Volume is not mounted, fails with error message as stated above.

Expected Results:  
Should not depend on ACLs being available."
Comment 1 David Zeuthen (not reading bugmail) 2012-04-20 05:41:06 UTC
setfacl(1) is used at runtime, there is no point in detecting if the build system has it - instead you want to add a runtime dep to your package spec file or similar. So technically this bug can be closed as INVALID.... however, since we really should just be using libattr instead of spawning a command, I'm repurposing this bug that - hope that's OK with you.
Comment 2 David Zeuthen (not reading bugmail) 2012-04-20 05:43:43 UTC
Actually, it's libacl, not libattr
Comment 3 Samuli Suominen 2012-04-20 06:01:56 UTC
(In reply to comment #1)
> setfacl(1) is used at runtime, there is no point in detecting if the build
> system has it - instead you want to add a runtime dep to your package spec file
> or similar. So technically this bug can be closed as INVALID.... however, since
> we really should just be using libattr instead of spawning a command, I'm
> repurposing this bug that - hope that's OK with you.

When adding udisks2, as a packager, I did various methods of tracking down the dependencies (including reading all autotools build system files, followed by scanelf for NEEDED entries, followed by various source tree greps, etc.
None of these turned setfacl command. It was only after users reported it to be failing at runtime.
So at very least, this should be documented in, eg. INSTALL doc file or somesuch.

What you described using the lib, instead of command, sounds fine to me

What about making it optional? Some fallback for systems with no acl/attr related tools/libs? This is not important to *me* but there are many people trying to keep system minimal as possible.

Overall, I'm fine with everything -- just long as it's more clear.
Comment 4 David Zeuthen (not reading bugmail) 2012-04-20 06:34:41 UTC
Fixed in master

 http://cgit.freedesktop.org/udisks/commit/?id=15250f35ff8770389cc579c304fbcac9beebc203
Comment 5 David Zeuthen (not reading bugmail) 2012-04-20 06:42:28 UTC
Btw, there's a bunch of other tools that we spawn at runtime including parted, sfdisk, sgdisk, mount, umount, cryptsetup, swapon, swapoff. It would be helpful to have this in README under a new section called RUNTIME DEPENDENCIES or something. Patches welcome - thanks!

(FWIW, I generally don't like making things dependencies optional - and in the setfacl/libacl case it really can't be made optional.)
Comment 6 Samuli Suominen 2012-04-20 10:09:57 UTC
(In reply to comment #5)
> Btw, there's a bunch of other tools that we spawn at runtime including parted,
> sfdisk, sgdisk, mount, umount, cryptsetup, swapon, swapoff. It would be helpful
> to have this in README under a new section called RUNTIME DEPENDENCIES or
> something. Patches welcome - thanks!
> 
> (FWIW, I generally don't like making things dependencies optional - and in the
> setfacl/libacl case it really can't be made optional.)

Thanks David, I'll see if I can come up with a nice list. Give me a day.