[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: modify chmod
From: |
Eric Blake |
Subject: |
Re: modify chmod |
Date: |
Fri, 05 Feb 2010 08:09:53 -0700 |
User-agent: |
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.23) Gecko/20090812 Thunderbird/2.0.0.23 Mnenhy/0.7.6.666 |
According to jeff.liu on 2/5/2010 7:44 AM:
> +++ b/gnulib
> @@ -1 +1 @@
> -Subproject commit 2eb5a8a0ff8348149a9ca985e2ccbfb03bc8de53
> +Subproject commit 4b93a2579fb567b9fbbeb24439770d814dac95cd
Why are you modifying the gnulib submodule?
>
> +/* Some systems only have <sys/vfs.h>, other systems also
> + * have <sys/statfs.h>, where the former includes the latter.
> + * So it seems including the former is the best choice. */
> +#include "fs.h"
> +#if HAVE_SYS_VFS_H
> +# include <sys/vfs.h>
> +#else
> +# include <sys/statfs.h>
> +#endif
Hmm. POSIX standardized <sys/statvfs.h>, not <sys/vfs.h>. Maybe a better
approach would be to propose a patch to gnulib that guarantees that
<sys/statvfs.h> provides everything we need, so that we don't have to ever
worry about <sys/vfs.h> in coreutils.
>
> +/* Return true if the file resides on NFS filesystem.
> + * limit this optimization to systems that provide statfs. */
This comment formatting is not consistent with other comments in the file.
> +
> +static bool
> +may_have_nfsacl(const char *file)
Missing a space between the function name and the (. And coreutils
prefers 'char const *' over the synonymous 'const char *'.
> +{
> +# if HAVE_SYS_VFS_H && HAVE_SYS_STATFS_H && HAVE_STRUCT_STATFS_F_TYPE
> + struct statfs buf;
> +
> + /* If statfs fails, assume we can't use the optimization. */
> + if (statfs (file, &buf) < 0)
> + return true;
> +
> + return buf.f_type == S_MAGIC_NFS;
> +#endif
> +
> + return true;
Rather than have #ifdefs inside the function body, this seems like it
would be better to do:
#ifdef ...
function
#else
# define may_have_nfsacl(ignored) true
#else
> +}
> +
> /* Change the mode of FILE.
> Return true if successful. This function is called
> once for every file system object that fts encounters. */
> @@ -257,18 +290,38 @@ process_file (FTS *fts, FTSENT *ent)
> old_mode = file_stats->st_mode;
> new_mode = mode_adjust (old_mode, S_ISDIR (old_mode) != 0, umask_value,
> change, NULL);
> -
> - if (! S_ISLNK (old_mode))
> +
> + /* Do not touch the inode if the new file mode is the same as it was
> before;
> + * This is an optimization for some cases. However, the new behavior
> must be
> + * disabled for filesystems that support NFSv4 ACLs.
> + * The idea suggested by Paul Eggert refer to FreeBSD chmod(1).
> + * it uses pathconf(2)/lpathconf(2) to determine whether this is the case.
> + * but on linux, we lack of them. Instead, calling statfs(2) and
> compare the
> + * f_type we can still do optimization at least its not NFS. */
Again, inconsistent comment formatting.
> + if (file_stats->st_uid != euid && euid != 0)
> + error (0, 0, _("changing permissions of %s: Operation not permitted"),
> + quote (file_full_name));
Indentation in this patch looks wrong; did it get munged? Also, typically
the "Operation not permitted" is provided for free by passing the
appropriate errno to error(), rather than open-coding it into the error
string.
--
Don't work too hard, make some time for fun as well!
Eric Blake address@hidden
signature.asc
Description: OpenPGP digital signature
- Re: modify chmod, Jim Meyering, 2010/02/01
- Re: modify chmod, jeff.liu, 2010/02/01
- Re: modify chmod, jeff.liu, 2010/02/05
- Re: modify chmod, Jim Meyering, 2010/02/06
- Re: modify chmod, jeff.liu, 2010/02/07
- Re: modify chmod, Jim Meyering, 2010/02/07
- Re: modify chmod, jeff.liu, 2010/02/07
- Re: modify chmod, Jim Meyering, 2010/02/07
- Re: modify chmod, jeff.liu, 2010/02/08
- Re: modify chmod, jeff.liu, 2010/02/08