Add the methods to change and get a posix file permission to file_util.

Add the methods to change and get a posix file permission to file_util.

BUG=134821, 134820
TEST=both base_unittests:FileUtilTest.* and unit_tests:Gdata* pass

[satorux1, 2012-07-02 16:07:35]

please split into two patches: 1) base changes 2) gdata changes. that'd be
cleaner.

btw, "Omit" may sound a bit unusual. "Get rid of" or "Eliminate" may be better,
but I'm not certain. Please try "git log" and see what others are saying on
changes like this. "Omit" may be ok.

[yoshiki, 2012-07-02 19:12:47]

brettw, estade: Could you take a look? I think you are familier with
file_util_posix.cc.

satorux: Thanks, I remove the gdata part from this changelist. It will be put on
an another change list.

[Evan Stade, 2012-07-03 01:41:06]

seems ok to me, but I am not an owner.

[satorux1, 2012-07-03 06:16:55]

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h#newcode200
base/file_util.h:200: // Read the permission of the given |path|. |mode| is file
permission bits.
Read -> Reads

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h#newcode200
base/file_util.h:200: // Read the permission of the given |path|. |mode| is file
permission bits.
Please also mention about how symbolic links are handled.

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h#newcode202
base/file_util.h:202: mode_t* mode);
Not sure if using mode_t is a good idea. we might want to define make it 'int'
and define easy-to-read enumerators like READ_BY_OTHERS than letting clients use
hard-to-read constants like  S_IROTH.

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h#newcode203
base/file_util.h:203: // Change the permission of the given |path|.
Change -> Changes

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h#newcode206
base/file_util.h:206: mode_t mode_bits_to_clear);
I think taking two parameters for chaning the permissions is rather awkward.
Would be better to be compatible with chmod(2), that only takes one parameter.

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc#new...
base/file_util_posix.cc:465: if (CallStat(path.value().c_str(), &file_info) !=
0)
i think there is a reason not to use lstat here. please add some comment

per http://linux.die.net/man/2/lstat, permission bits in st_mode are invalid
with lstat(2):


According to POSIX.1-2001, lstat() on a symbolic link need return valid
information only in the st_size field and the file-type component of the st_mode
field of the stat structure. POSIX.-2008 tightens the specification, requiring
lstat() to return valid information in all fields except the permission bits in
st_mode.

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc#new...
base/file_util_posix.cc:480: if (CallLstat(path.value().c_str(), &stat_buf) !=
0)
why lstat? it seems lstat on symbolic link is invalid

[satorux1, 2012-07-03 06:22:36]

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc#new...
base/file_util_posix.cc:480: if (CallLstat(path.value().c_str(), &stat_buf) !=
0)
On 2012/07/03 06:16:55, satorux1 wrote:
> why lstat? it seems lstat on symbolic link is invalid

invalid for permission bits

[satorux1, 2012-07-03 06:23:29]

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc#new...
base/file_util_posix.cc:487: if (chmod(path.value().c_str(), updated_mode_bits)
!= 0)
please use HANDLE_EINTR when you call a system call

[yoshiki, 2012-07-04 07:57:57]

brettw: could you take a look and approve check-in?

satorux, estade: Thanks for review!

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h#newcode200
base/file_util.h:200: // Read the permission of the given |path|. |mode| is file
permission bits.
On 2012/07/03 06:16:55, satorux1 wrote:
> Read -> Reads

Done x2.

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h#newcode202
base/file_util.h:202: mode_t* mode);
On 2012/07/03 06:16:55, satorux1 wrote:
> Not sure if using mode_t is a good idea. we might want to define make it 'int'
> and define easy-to-read enumerators like READ_BY_OTHERS than letting clients
use
> hard-to-read constants like  S_IROTH. 

Done.

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h#newcode203
base/file_util.h:203: // Change the permission of the given |path|.
On 2012/07/03 06:16:55, satorux1 wrote:
> Change -> Changes

Done.

http://codereview.chromium.org/10696069/diff/9001/base/file_util.h#newcode206
base/file_util.h:206: mode_t mode_bits_to_clear);
On 2012/07/03 06:16:55, satorux1 wrote:
> I think taking two parameters for chaning the permissions is rather awkward.
> Would be better to be compatible with chmod(2), that only takes one parameter.

Done. I added another method SetPosixFilePermissions().

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc#new...
base/file_util_posix.cc:465: if (CallStat(path.value().c_str(), &file_info) !=
0)
On 2012/07/03 06:16:55, satorux1 wrote:
> i think there is a reason not to use lstat here. please add some comment
> 
> per http://linux.die.net/man/2/lstat, permission bits in st_mode are invalid
> with lstat(2):
> 
> 
> According to POSIX.1-2001, lstat() on a symbolic link need return valid
> information only in the st_size field and the file-type component of the
st_mode
> field of the stat structure. POSIX.-2008 tightens the specification, requiring
> lstat() to return valid information in all fields except the permission bits
in
> st_mode.

Done.

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc#new...
base/file_util_posix.cc:480: if (CallLstat(path.value().c_str(), &stat_buf) !=
0)
Sorry, I made a mistake.

On 2012/07/03 06:16:55, satorux1 wrote:
> why lstat? it seems lstat on symbolic link is invalid

http://codereview.chromium.org/10696069/diff/9001/base/file_util_posix.cc#new...
base/file_util_posix.cc:487: if (chmod(path.value().c_str(), updated_mode_bits)
!= 0)
On 2012/07/03 06:23:29, satorux1 wrote:
> please use HANDLE_EINTR when you call a system call

Done.

[satorux1, 2012-07-04 16:10:41]

http://codereview.chromium.org/10696069/diff/13002/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10696069/diff/13002/base/file_util.h#newcode205
base/file_util.h:205: FILE_PERMISSION_OTHERS_MASK       = 00007,
Not sure if thees masks are needed. maybe remove?

http://codereview.chromium.org/10696069/diff/13002/base/file_util.h#newcode207
base/file_util.h:207: FILE_PERMISSION_READ_BY_USER      = 00400,
Use sys/stat.h constants?
FILE_PERMISSION_READ_BY_USER = S_IRUSR

http://codereview.chromium.org/10696069/diff/13002/base/file_util.h#newcode229
base/file_util.h:229: int mode_bits_to_clear);
This function seems to be unnecessary as public API. Move this to the
unittest.cc file?

[yoshiki, 2012-07-09 23:44:10]

Fixed reviews, but something error happens and I can't upload the patch anymore.
So I've closed this and created a new issue:
https://chromiumcodereview.appspot.com/10756020/

http://codereview.chromium.org/10696069/diff/13002/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10696069/diff/13002/base/file_util.h#newcode205
base/file_util.h:205: FILE_PERMISSION_OTHERS_MASK       = 00007,
These are used at file_util_unittest.cc. Keeping.

On 2012/07/04 16:10:41, satorux1 wrote:
> Not sure if thees masks are needed. maybe remove?

http://codereview.chromium.org/10696069/diff/13002/base/file_util.h#newcode207
base/file_util.h:207: FILE_PERMISSION_READ_BY_USER      = 00400,
On 2012/07/04 16:10:41, satorux1 wrote:
> Use sys/stat.h constants?
> FILE_PERMISSION_READ_BY_USER = S_IRUSR

Done.

http://codereview.chromium.org/10696069/diff/13002/base/file_util.h#newcode229
base/file_util.h:229: int mode_bits_to_clear);
On 2012/07/04 16:10:41, satorux1 wrote:
> This function seems to be unnecessary as public API. Move this to the
> unittest.cc file?

Done.