Re: modify chmod

[Eric Blake]

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