Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10756020:
Add the methods to change and get a posix file permission to file_util. (Closed)
|
Created:
8 years, 5 months ago by yoshiki Modified:
8 years, 5 months ago CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd the methods to change and get a posix file permission to file_util.
BUG=134821, 134820
TEST=both base_unittests:FileUtilTest.*, VerifyPathControlledByUserTest.* and unit_tests:Gdata* pass
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146020
Patch Set 1 #
Total comments: 40
Patch Set 2 : Review fix #
Total comments: 8
Patch Set 3 : Review fix #
Messages
Total messages: 8 (0 generated)
brettw, satorux: could you take a look? This CL is moved from http://codereview.chromium.org/10696069/ because of some errors at Rietveld.
http://codereview.chromium.org/10756020/diff/1/base/file_util.h File base/file_util.h (right): http://codereview.chromium.org/10756020/diff/1/base/file_util.h#newcode104 base/file_util.h:104: // the symlink. (even if the symlink deferences to a non-existent file) while you are at it, please s/deferences/points/ http://codereview.chromium.org/10756020/diff/1/base/file_util.h#newcode202 base/file_util.h:202: FILE_PERMISSION_MASK = S_IRWXU | S_IRWXU | S_IRWXO, -> S_IRWXU | S_IRWXG | S_IRWXO http://codereview.chromium.org/10756020/diff/1/base/file_util.h#newcode220 base/file_util.h:220: // a file which the symlink refers to. refers to -> points to http://codereview.chromium.org/10756020/diff/1/base/file_util.h#newcode223 base/file_util.h:223: // Sets the permission of the given |path|. If |path| is symbolic link, ... http://codereview.chromium.org/10756020/diff/1/base/file_util_posix.cc File base/file_util_posix.cc (right): http://codereview.chromium.org/10756020/diff/1/base/file_util_posix.cc#newcod... base/file_util_posix.cc:465: // Uses stat(), because on symbolic link, lstat() may not return valid may not -> does not http://codereview.chromium.org/10756020/diff/1/base/file_util_posix.cc#newcod... base/file_util_posix.cc:466: // information in the permission bits st_mode. information in the permission bits st_mode. -> permission bits in st_mode http://codereview.chromium.org/10756020/diff/1/base/file_util_posix.cc#newcod... base/file_util_posix.cc:481: return false; Why should we get the current permission? This looks unnecessary. Cannot we just call chmod()? chmod() will fail if the file is not present anyway. http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc File base/file_util_unittest.cc (right): http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:118: DCHECK((mode_bits_to_clear & ~file_util::FILE_PERMISSION_MASK) == 0); the two lines look unnecessary, as we have a DCHECK in SetPosixFilePermissions() anway http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:122: int mode; mode = 0; just in case. http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:129: if (file_util::SetPosixFilePermissions(path, mode)) Is this correct? Shouldn't we have ! here? if (!file_util::SetPosixFilePermissions(path, mode)) http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:790: // Create a file path period is missing. http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:794: char buffer[32] = "hello"; remove 32. even better, remove this variable. http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:795: std::string data(buffer); const std::string kData = "hello"; http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:803: int32 mode; int mode = 0; http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:806: mode & file_util::FILE_PERMISSION_READ_BY_USER); EXPECT_TRUE(mode & file_util::FILE_PERMISSION_READ_BY_USER) http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:811: EXPECT_EQ(0, mode & file_util::FILE_PERMISSION_READ_BY_USER); EXPECT_FALSE(mode & file_util::FILE_PERMISSION_READ_BY_USER) http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:813: EXPECT_EQ(-1, file_util::ReadFile(file_name, buffer, sizeof(buffer))); kData.data(), kData.size()
https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util.h File base/file_util.h (right): https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util.h#newco... base/file_util.h:104: // the symlink. (even if the symlink deferences to a non-existent file) On 2012/07/10 00:06:54, satorux1 wrote: > while you are at it, please s/deferences/points/ Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util.h#newco... base/file_util.h:202: FILE_PERMISSION_MASK = S_IRWXU | S_IRWXU | S_IRWXO, On 2012/07/10 00:06:54, satorux1 wrote: > -> S_IRWXU | S_IRWXG | S_IRWXO Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util.h#newco... base/file_util.h:220: // a file which the symlink refers to. On 2012/07/10 00:06:54, satorux1 wrote: > refers to -> points to Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util.h#newco... base/file_util.h:223: // Sets the permission of the given |path|. On 2012/07/10 00:06:54, satorux1 wrote: > If |path| is symbolic link, ... Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_posix.cc File base/file_util_posix.cc (right): https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_posix.c... base/file_util_posix.cc:465: // Uses stat(), because on symbolic link, lstat() may not return valid On 2012/07/10 00:06:54, satorux1 wrote: > may not -> does not Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_posix.c... base/file_util_posix.cc:466: // information in the permission bits st_mode. On 2012/07/10 00:06:54, satorux1 wrote: > information in the permission bits st_mode. -> > permission bits in st_mode Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_posix.c... base/file_util_posix.cc:481: return false; This obtains the higher bits (S_ISUID, S_ISGID and S_ISVTX). SetPosixFilePermissions() accept only lower bits (user, group and others permission bits), so we have to perserve the other bits. On 2012/07/10 00:06:54, satorux1 wrote: > Why should we get the current permission? This looks unnecessary. > > Cannot we just call chmod()? chmod() will fail if the file is not present > anyway. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... File base/file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:118: DCHECK((mode_bits_to_clear & ~file_util::FILE_PERMISSION_MASK) == 0); On 2012/07/10 00:06:54, satorux1 wrote: > the two lines look unnecessary, as we have a DCHECK in SetPosixFilePermissions() > anway Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:122: int mode; On 2012/07/10 00:06:54, satorux1 wrote: > mode = 0; just in case. Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:129: if (file_util::SetPosixFilePermissions(path, mode)) Oops... Done. On 2012/07/10 00:06:54, satorux1 wrote: > Is this correct? Shouldn't we have ! here? > > if (!file_util::SetPosixFilePermissions(path, mode)) https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:790: // Create a file path On 2012/07/10 00:06:54, satorux1 wrote: > period is missing. Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:794: char buffer[32] = "hello"; On 2012/07/10 00:06:54, satorux1 wrote: > remove 32. even better, remove this variable. Removed 32. But not remove the variable because it is used later. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:795: std::string data(buffer); Made it const and changed name, keeping the initialization from |buffer| since it is not removed. On 2012/07/10 00:06:54, satorux1 wrote: > const std::string kData = "hello"; https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:803: int32 mode; On 2012/07/10 00:06:54, satorux1 wrote: > int mode = 0; Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:806: mode & file_util::FILE_PERMISSION_READ_BY_USER); On 2012/07/10 00:06:54, satorux1 wrote: > EXPECT_TRUE(mode & file_util::FILE_PERMISSION_READ_BY_USER) Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:811: EXPECT_EQ(0, mode & file_util::FILE_PERMISSION_READ_BY_USER); On 2012/07/10 00:06:54, satorux1 wrote: > EXPECT_FALSE(mode & file_util::FILE_PERMISSION_READ_BY_USER) Done. https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:813: EXPECT_EQ(-1, file_util::ReadFile(file_name, buffer, sizeof(buffer))); string::data() returns a pointer of const char*, so it is unsafe to write to the pointer. On 2012/07/10 00:06:54, satorux1 wrote: > kData.data(), kData.size()
LGTM with nits: http://codereview.chromium.org/10756020/diff/1/base/file_util.h File base/file_util.h (right): http://codereview.chromium.org/10756020/diff/1/base/file_util.h#newcode104 base/file_util.h:104: // the symlink. (even if the symlink deferences to a non-existent file) On 2012/07/10 01:31:17, yoshiki wrote: > On 2012/07/10 00:06:54, satorux1 wrote: > > while you are at it, please s/deferences/points/ > > Done. point -> points http://codereview.chromium.org/10756020/diff/1/base/file_util.h#newcode220 base/file_util.h:220: // a file which the symlink refers to. On 2012/07/10 01:31:17, yoshiki wrote: > On 2012/07/10 00:06:54, satorux1 wrote: > > refers to -> points to > > Done. points http://codereview.chromium.org/10756020/diff/1/base/file_util_posix.cc File base/file_util_posix.cc (right): http://codereview.chromium.org/10756020/diff/1/base/file_util_posix.cc#newcod... base/file_util_posix.cc:481: return false; On 2012/07/10 01:31:17, yoshiki wrote: > This obtains the higher bits (S_ISUID, S_ISGID and S_ISVTX). > SetPosixFilePermissions() accept only lower bits (user, group and others > permission bits), so we have to perserve the other bits. Oh i see. Then please leave some comment like: // Call stat() so that we can preserve the higher bits like S_ISGID. > > On 2012/07/10 00:06:54, satorux1 wrote: > > Why should we get the current permission? This looks unnecessary. > > > > Cannot we just call chmod()? chmod() will fail if the file is not present > > anyway. > http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc File base/file_util_unittest.cc (right): http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:795: std::string data(buffer); Reusing 'buffer' at a later time for writing sounds a bit unclean. Maybe: const std::string kData("hello"); ... std::string buffer(kData.length()); EXPECT_EQ(static_cast<int>(kData.length()), file_util::ReadFile(file_name, string_as_array(buffer), buffer.size()); However it's not a big deal so it's up to you. http://codereview.chromium.org/10756020/diff/1/base/file_util_unittest.cc#new... base/file_util_unittest.cc:813: EXPECT_EQ(-1, file_util::ReadFile(file_name, buffer, sizeof(buffer))); oops, you are right. On 2012/07/10 01:31:17, yoshiki wrote: > string::data() returns a pointer of const char*, so it is unsafe to write to the > pointer. > > On 2012/07/10 00:06:54, satorux1 wrote: > > kData.data(), kData.size() > http://codereview.chromium.org/10756020/diff/14001/base/file_util.h File base/file_util.h (right): http://codereview.chromium.org/10756020/diff/14001/base/file_util.h#newcode224 base/file_util.h:224: // the permission of a file which the symlink point to. points http://codereview.chromium.org/10756020/diff/14001/base/file_util_posix.cc File base/file_util_posix.cc (right): http://codereview.chromium.org/10756020/diff/14001/base/file_util_posix.cc#ne... base/file_util_posix.cc:482: might want to add // Clear the existing permission bits, and add the new ones. http://codereview.chromium.org/10756020/diff/14001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): http://codereview.chromium.org/10756020/diff/14001/base/file_util_unittest.cc... base/file_util_unittest.cc:114: void ChangePosixFilePermissions(const FilePath& path, function comment is missing. maybe restore the original function comment? http://codereview.chromium.org/10756020/diff/14001/base/file_util_unittest.cc... base/file_util_unittest.cc:791: file_util::WriteFile(file_name, kData.c_str(), kData.length())); c_str() -> data(), as you don't need \0 at the end when you pass with the length.
brettw: Could you take a look? satorux: Thanks for comments! https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... File base/file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/10756020/diff/1/base/file_util_unittes... base/file_util_unittest.cc:795: std::string data(buffer); I think direct manipulation to buffer of std::string is not common. I change the code to alloc char array buffer dynamically. On 2012/07/10 06:57:17, satorux1 wrote: > Reusing 'buffer' at a later time for writing sounds a bit unclean. Maybe: > > const std::string kData("hello"); > ... > std::string buffer(kData.length()); > EXPECT_EQ(static_cast<int>(kData.length()), > file_util::ReadFile(file_name, string_as_array(buffer), > buffer.size()); > > However it's not a big deal so it's up to you. https://chromiumcodereview.appspot.com/10756020/diff/14001/base/file_util.h File base/file_util.h (right): https://chromiumcodereview.appspot.com/10756020/diff/14001/base/file_util.h#n... base/file_util.h:224: // the permission of a file which the symlink point to. On 2012/07/10 06:57:17, satorux1 wrote: > points Done. https://chromiumcodereview.appspot.com/10756020/diff/14001/base/file_util_pos... File base/file_util_posix.cc (right): https://chromiumcodereview.appspot.com/10756020/diff/14001/base/file_util_pos... base/file_util_posix.cc:482: On 2012/07/10 06:57:17, satorux1 wrote: > might want to add > > // Clear the existing permission bits, and add the new ones. Done. https://chromiumcodereview.appspot.com/10756020/diff/14001/base/file_util_uni... File base/file_util_unittest.cc (right): https://chromiumcodereview.appspot.com/10756020/diff/14001/base/file_util_uni... base/file_util_unittest.cc:114: void ChangePosixFilePermissions(const FilePath& path, On 2012/07/10 06:57:17, satorux1 wrote: > function comment is missing. maybe restore the original function comment? Done. https://chromiumcodereview.appspot.com/10756020/diff/14001/base/file_util_uni... base/file_util_unittest.cc:791: file_util::WriteFile(file_name, kData.c_str(), kData.length())); On 2012/07/10 06:57:17, satorux1 wrote: > c_str() -> data(), as you don't need \0 at the end when you pass with the > length. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10756020/19001
Change committed as 146020 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
