Re: modify chmod

[jeff.liu]

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?
>
>   
Hmm, I also wonder what's the reason, maybe caused by my another code
tryout.
>> +/* 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.
>
>   
Let me do more investigation for gnulib.
>> +/* 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 *'.
>
>   
I still need to get familiar with the GNU code style.
>> +{
>> +# 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
>
>   
Really a good idea.
>> +}
>> +
>> /* 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.
>
>   
Thanks for the point out, learn from your comments again!