Date: Wed, 9 Feb 2011 14:10:42 -0700 From: Shawn Webb <lattera@gmail.com> To: Tim Kientzle <tim@kientzle.com> Cc: FreeBSD-current <freebsd-current@freebsd.org> Subject: Re: setfacl Recursive Functionality Message-ID: <AANLkTim9anqAqSLH=5OMi_dTyAsqY2xCL37KM8MmrZdR@mail.gmail.com> In-Reply-To: <AANLkTin2XR9Ra4LK-rgh94XaAUB6jY_sg0n51GdbgnQ2@mail.gmail.com> References: <AANLkTi=%2BWtmRz07m=Cg7hbXJGw7eWRHC1ASGeufTSLBB@mail.gmail.com> <80373F51-25C7-48A0-8920-3444A98D857F@kientzle.com> <AANLkTin2XR9Ra4LK-rgh94XaAUB6jY_sg0n51GdbgnQ2@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] Included in the attached patch is the refactor using fts(3) and with the -L and -H options. I'm still looking for feedback and suggestions on how to improve the patch. I'll also port these changes over to my getfacl patch. If anyone's interested in following up-to-date development of the patch, the link to it on github is https://github.com/lattera/patches/blob/master/freebsd/setfacl_recursive.patch I'd like to take the time to address why I created the remove_invalid_inherit function since I got a private email asking why it existed. Other than symbolic links, non-directory entries cannot have inheritance set. That function prevents attempting to set inheritance flags on non-directory entries when doing a recursive call. That way, you can run `setfacl -R -m user:<user>:read_data:file_inherit/dir_inherit:allow <directory>` and not run into errors. Thanks, Shawn On Tue, Feb 8, 2011 at 7:51 PM, Shawn Webb <lattera@gmail.com> wrote: > On Tue, Feb 8, 2011 at 7:35 PM, Tim Kientzle <tim@kientzle.com> wrote: > >> On Feb 8, 2011, at 9:58 AM, Shawn Webb wrote: >> > I've just finished a patch to add recursive functionality to setfacl. >> Before >> > I officially submit it, I'd like a few suggestions on how to improve the >> > patch. >> > >> > The part I'm worried about involves the #define directive at top. I'm >> not >> > sure what ramifications using that define might have. I needed it for my >> > remove_invalid_inherit() function to work. >> >> You should certainly not need >> #define _ACL_PRIVATE >> for any user-space utilities. What exactly is the >> problem without that? >> >> Your approach to directory walking here >> is a little simplistic. In particular, you're storing >> every filename for the entire tree in memory, >> which is a problem for large filesystems. >> >> It would be much better to refactor the code so that >> the actual ACL update was in a function and then >> recurse_directory should call that function for >> each filename as it visited it. That will reduce >> the memory requirements significantly. >> >> You should also take a look at fts(3). In particular, >> you'll want to implement the BSD-standard >> -L/-P/-H options, and fts(3) makes that much easier. >> (-L always follows symlinks, -P never follows symlinks, >> -H follows symlinks on the command line). >> >> Tim >> >> > Great suggestions. I'll definitely look at implementing that functionality. > > As a side note, it looks like my setfacl patch segfaults on freebsd-current > r218075 with the zpool v28 patchset applied. I wrote it on freebsd 8.2-RC3 > with zpool v15. > > Thanks, > > Shawn > [-- Attachment #2 --] --- /usr/src/bin/setfacl/setfacl.c 2011-02-03 12:11:02.303496318 -0700 +++ bin/setfacl/setfacl.c 2011-02-09 13:59:06.961538056 -0700 @@ -23,7 +23,7 @@ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE * POSSIBILITY OF SUCH DAMAGE. */ - +#define _ACL_PRIVATE #include <sys/cdefs.h> __FBSDID("$FreeBSD$"); @@ -32,6 +32,8 @@ #include <sys/stat.h> #include <sys/acl.h> #include <sys/queue.h> +#include <fts.h> +#include <dirent.h> #include <err.h> #include <errno.h> @@ -44,6 +46,8 @@ static void add_filename(const char *filename); static void usage(void); +static void recurse_directory(char *const *paths, int r_flag, int l_flag, int big_h_flag); +static acl_t remove_invalid_inherit(const char *path, acl_t acl, int l_flag); static void add_filename(const char *filename) @@ -62,34 +66,106 @@ static void usage(void) { - - fprintf(stderr, "usage: setfacl [-bdhkn] [-a position entries] " + fprintf(stderr, "usage: setfacl [-bdhkLnR] [-a position entries] " "[-m entries] [-M file] [-x entries] [-X file] [file ...]\n"); exit(1); } +static void +recurse_directory(char *const *paths, int r_flag, int l_flag, int big_h_flag) +{ + FTS *ftsp; + FTSENT *p, *chp; + int fts_options = FTS_NOCHDIR; + unsigned int i; + + fts_options |= (l_flag == 1) ? FTS_LOGICAL : FTS_PHYSICAL; + if (big_h_flag) + fts_options |= FTS_COMFOLLOW; + + if (r_flag) + { + ftsp = fts_open(paths, fts_options, NULL); + if (ftsp == NULL) + return; + + chp = fts_children(ftsp, 0); + if (chp == NULL) + return; + + while ((p = fts_read(ftsp)) != NULL) { + if (l_flag == 0 && p->fts_info & FTS_D) + continue; + else if (l_flag == 1 && p->fts_info & FTS_DP) + continue; + + add_filename(strdup(p->fts_path)); + } + + fts_close(ftsp); + } else + for (i = 0; paths[i] != NULL; i++) + add_filename(paths[i]); +} + +static acl_t +remove_invalid_inherit(const char *path, acl_t acl, int l_flag) +{ + acl_t acl_new; + int acl_brand; + acl_entry_t entry; + int entry_id; + struct stat sb; + + acl_get_brand_np(acl, &acl_brand); + if (acl_brand != ACL_BRAND_NFS4) + return acl; + + if (l_flag == 1) { + if (stat(path, &sb) == -1) + return acl; + } else + if (lstat(path, &sb) == -1) + return acl; + + if (S_ISDIR(sb.st_mode) != 0) + return acl; + + acl_new = acl_dup(acl); + + entry_id = ACL_FIRST_ENTRY; + while (acl_get_entry(acl_new, entry_id, &entry) == 1) { + entry_id = ACL_NEXT_ENTRY; + entry->ae_flags = 0; + } + + return acl_new; +} + int main(int argc, char *argv[]) { - acl_t acl; + acl_t acl, acl_backup; acl_type_t acl_type; char filename[PATH_MAX]; - int local_error, carried_error, ch, i, entry_number, ret; - int h_flag; + int local_error, carried_error, ch, entry_number, ret; + int h_flag, r_flag, l_flag, big_h_flag; struct sf_file *file; struct sf_entry *entry; - const char *fn_dup; + char *fn_dup; char *end; + char **files=NULL; + unsigned int numfiles=0; struct stat sb; acl_type = ACL_TYPE_ACCESS; carried_error = local_error = 0; - h_flag = have_mask = have_stdin = n_flag = need_mask = 0; + h_flag = have_mask = have_stdin = n_flag = need_mask = r_flag = l_flag = big_h_flag = 0; TAILQ_INIT(&entrylist); TAILQ_INIT(&filelist); - while ((ch = getopt(argc, argv, "M:X:a:bdhkm:nx:")) != -1) + while ((ch = getopt(argc, argv, "HLRM:X:a:bdhkm:nx:")) != -1) switch(ch) { case 'M': entry = zmalloc(sizeof(struct sf_entry)); @@ -167,6 +243,15 @@ } TAILQ_INSERT_TAIL(&entrylist, entry, next); break; + case 'R': + r_flag = 1; + break; + case 'L': + l_flag = 1; + break; + case 'H': + big_h_flag = 1; + break; default: usage(); break; @@ -189,11 +274,18 @@ fn_dup = strdup(filename); if (fn_dup == NULL) err(1, "strdup() failed"); - add_filename(fn_dup); + files = realloc(files, ++numfiles * sizeof(char **)); + if (files == NULL) + err(1, "realloc() failed"); + files[numfiles-1] = (char *)fn_dup; } + + files = realloc(files, ++numfiles * sizeof(char **)); + files[numfiles-1] = NULL; } else - for (i = 0; i < argc; i++) - add_filename(argv[i]); + files = argv; + + recurse_directory(files, r_flag, l_flag, big_h_flag); /* cycle through each file */ TAILQ_FOREACH(file, &filelist, next) { @@ -250,12 +342,24 @@ switch(entry->op) { case OP_ADD_ACL: + acl_backup = entry->acl; + entry->acl = remove_invalid_inherit(file->filename, entry->acl, l_flag); local_error += add_acl(entry->acl, entry->entry_number, &acl, file->filename); + if (entry->acl != acl_backup) { + acl_free(entry->acl); + entry->acl = acl_backup; + } break; case OP_MERGE_ACL: + acl_backup = entry->acl; + entry->acl = remove_invalid_inherit(file->filename, entry->acl, l_flag); local_error += merge_acl(entry->acl, &acl, file->filename); + if (entry->acl != acl_backup) { + acl_free(entry->acl); + entry->acl = acl_backup; + } need_mask = 1; break; case OP_REMOVE_EXT:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTim9anqAqSLH=5OMi_dTyAsqY2xCL37KM8MmrZdR>