Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
791493009:
Add wrappers around getxattr() and setxattr(). (Closed)
|
Created:
6 years ago by Robert Sesek Modified:
5 years, 11 months ago Reviewers:
Mark Mentovai CC:
crashpad-dev_chromium.org Base URL:
https://chromium.googlesource.com/crashpad/crashpad@master Target Ref:
refs/heads/master Project:
crashpad Visibility:
Public. |
DescriptionAdd wrappers around getxattr() and setxattr().
R=mark@chromium.org
TEST=util_test --gtest_filter=Xattr.*
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/8e98c9251a2bfa276e2943eaccbfe1faba955d16
Patch Set 1 #Patch Set 2 : #
Total comments: 37
Patch Set 3 : Address comments #
Total comments: 11
Patch Set 4 : Address comments #
Total comments: 6
Patch Set 5 : For landing #
Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc File util/mac/xattr.cc (right): https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode36 util/mac/xattr.cc:36: rv = getxattr(file.value().c_str(), name.data(), &(*value)[0], I don’t like the new way clang-format wants us to write parameters, but I won’t force you to change it if you like it. Same on line 58. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode37 util/mac/xattr.cc:37: value->capacity(), 0, 0); You need to use value->size() and not value->capacity(). If the capacity is larger than the size, then you have enough space to read the xattr, but when you subsequently resize() it, the portion beyond size() will be zeroed out. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode39 util/mac/xattr.cc:39: if (errno == ERANGE) { #include <errno.h> https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode40 util/mac/xattr.cc:40: value->resize(value->capacity() * 2); Where’s the upper-bound protection? https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode41 util/mac/xattr.cc:41: continue; Get rid of this continue and put the PLOG/return that follows into an else. The condition of the loop at the bottom is already testing rv < 0 && errno == ERANGE. Or, leave this alone right here and change the condition down below to false, relying only on the continue to cause the loop to run for another iteration. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode44 util/mac/xattr.cc:44: PLOG(ERROR) << "ReadXattr " << name << " on file " << file.value(); PLOGs normally show the name of the syscall, not the name of their own function, so this should be getxattr instead of ReadXattr. Similar on line 60. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode78 util/mac/xattr.cc:78: return false; You need to log a message here to maintain the documented API contract. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode85 util/mac/xattr.cc:85: return WriteXattr(file, name, (value ? "1" : "0")); The inner () aren’t necessary. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcod... util/mac/xattr.cc:117: *value = base::saturated_cast<time_t, int64_t>(encoded_value); I think this should log a warning if it saturates. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.h File util/mac/xattr.h (right): https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.h#newcode31 util/mac/xattr.h:31: //! \return True if the read was successful, with \a value filled in. False on `true` and `false`. Line 43 too. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.h#newcode35 util/mac/xattr.h:35: std::string* value); #include <string> https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.h#newcode49 util/mac/xattr.h:49: //! \copydoc ReadXattr Append something to this saying that you only consider 0 to be false and 1 to be true, and anything else is considered an error. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc File util/mac/xattr_test.cc (right): https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:1: // Copyright 2014 The Crashpad Authors. All rights reserved. CL description: TEST= https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:33: void SetUp() override { // testing::Test: to tell what it’s overriding (although in this case, it’s obvious, since there’s only one base.) https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:35: base::StringPrintf("/tmp/com.google.crashpad.test.xattr.%p", this)); org.googlecode.crashpad https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:36: base::ScopedFD tmp(open(path_.value().c_str(), O_CREAT | O_TRUNC, S_IRWXU)); You don’t want the x-bit. S_IRUSR | S_IWUSR. I’d probably just write 0644 and let the umask sort it out. You must use O_EXCL. You must use HANDLE_EINTR and #include the proper header. You should leave behind a TODO or a bug reference saying that this really wants ScopedTempDir. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:37: EXPECT_GE(tmp.get(), 0) << errno; #include "util/test/errros.h" and use << ErrnoMessage("open"); Similar on line 41. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:67: std::string value(512, 'A'); Maybe a not-power-of-2-aligned-length string?
https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc File util/mac/xattr.cc (right): https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode36 util/mac/xattr.cc:36: rv = getxattr(file.value().c_str(), name.data(), &(*value)[0], On 2014/12/19 23:16:32, Mark Mentovai wrote: > I don’t like the new way clang-format wants us to write parameters, but I won’t > force you to change it if you like it. Same on line 58. What's wrong with this? (I didn't clang-format but handwrote it). https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode37 util/mac/xattr.cc:37: value->capacity(), 0, 0); On 2014/12/19 23:16:32, Mark Mentovai wrote: > You need to use value->size() and not value->capacity(). If the capacity is > larger than the size, then you have enough space to read the xattr, but when you > subsequently resize() it, the portion beyond size() will be zeroed out. Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode39 util/mac/xattr.cc:39: if (errno == ERANGE) { On 2014/12/19 23:16:32, Mark Mentovai wrote: > #include <errno.h> Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode40 util/mac/xattr.cc:40: value->resize(value->capacity() * 2); On 2014/12/19 23:16:32, Mark Mentovai wrote: > Where’s the upper-bound protection? I initially wrote upper-bound protection but later removed it. What would a sensible value be? https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode41 util/mac/xattr.cc:41: continue; On 2014/12/19 23:16:32, Mark Mentovai wrote: > Get rid of this continue and put the PLOG/return that follows into an else. > > The condition of the loop at the bottom is already testing rv < 0 && errno == > ERANGE. > > Or, leave this alone right here and change the condition down below to false, > relying only on the continue to cause the loop to run for another iteration. Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode44 util/mac/xattr.cc:44: PLOG(ERROR) << "ReadXattr " << name << " on file " << file.value(); On 2014/12/19 23:16:32, Mark Mentovai wrote: > PLOGs normally show the name of the syscall, not the name of their own function, > so this should be getxattr instead of ReadXattr. Similar on line 60. Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode78 util/mac/xattr.cc:78: return false; On 2014/12/19 23:16:31, Mark Mentovai wrote: > You need to log a message here to maintain the documented API contract. Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode85 util/mac/xattr.cc:85: return WriteXattr(file, name, (value ? "1" : "0")); On 2014/12/19 23:16:32, Mark Mentovai wrote: > The inner () aren’t necessary. Indeed, but I think they help readability here. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcod... util/mac/xattr.cc:117: *value = base::saturated_cast<time_t, int64_t>(encoded_value); On 2014/12/19 23:16:32, Mark Mentovai wrote: > I think this should log a warning if it saturates. Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.h File util/mac/xattr.h (right): https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.h#newcode31 util/mac/xattr.h:31: //! \return True if the read was successful, with \a value filled in. False on On 2014/12/19 23:16:32, Mark Mentovai wrote: > `true` and `false`. Line 43 too. Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.h#newcode35 util/mac/xattr.h:35: std::string* value); On 2014/12/19 23:16:32, Mark Mentovai wrote: > #include <string> Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.h#newcode49 util/mac/xattr.h:49: //! \copydoc ReadXattr On 2014/12/19 23:16:32, Mark Mentovai wrote: > Append something to this saying that you only consider 0 to be false and 1 to be > true, and anything else is considered an error. Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc File util/mac/xattr_test.cc (right): https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:1: // Copyright 2014 The Crashpad Authors. All rights reserved. On 2014/12/19 23:16:32, Mark Mentovai wrote: > CL description: TEST= Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:33: void SetUp() override { On 2014/12/19 23:16:32, Mark Mentovai wrote: > // testing::Test: to tell what it’s overriding (although in this case, it’s > obvious, since there’s only one base.) Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:35: base::StringPrintf("/tmp/com.google.crashpad.test.xattr.%p", this)); On 2014/12/19 23:16:32, Mark Mentovai wrote: > org.googlecode.crashpad Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:36: base::ScopedFD tmp(open(path_.value().c_str(), O_CREAT | O_TRUNC, S_IRWXU)); On 2014/12/19 23:16:32, Mark Mentovai wrote: > You don’t want the x-bit. S_IRUSR | S_IWUSR. I’d probably just write 0644 and > let the umask sort it out. > > You must use O_EXCL. > > You must use HANDLE_EINTR and #include the proper header. > > You should leave behind a TODO or a bug reference saying that this really wants > ScopedTempDir. Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:37: EXPECT_GE(tmp.get(), 0) << errno; On 2014/12/19 23:16:32, Mark Mentovai wrote: > #include "util/test/errros.h" and use << ErrnoMessage("open"); > > Similar on line 41. Done. https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:67: std::string value(512, 'A'); On 2014/12/19 23:16:32, Mark Mentovai wrote: > Maybe a not-power-of-2-aligned-length string? Done.
LGTM https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc File util/mac/xattr.cc (right): https://codereview.chromium.org/791493009/diff/20001/util/mac/xattr.cc#newcode36 util/mac/xattr.cc:36: rv = getxattr(file.value().c_str(), name.data(), &(*value)[0], On 2014/12/30 17:02:21, Robert Sesek wrote: > On 2014/12/19 23:16:32, Mark Mentovai wrote: > > I don’t like the new way clang-format wants us to write parameters, but I > won’t > > force you to change it if you like it. Same on line 58. > > What's wrong with this? (I didn't clang-format but handwrote it). Three parameters on one line, three on the other, no real alignment to show that they’re all parameters of equal importance, no real thought put into grouping similar parameters… Assuming everything doesn’t all fit on a single line, I usually do: SomeFunction( some_argument, some_other_argument, some_more_argumentative_stuff); AnotherFunction(allays, eye, bee, eff); We recently did https://codereview.chromium.org/805333006/ after discussing at https://codereview.chromium.org/821483002/diff/20001/util/file/file_io_posix..... https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.cc File util/mac/xattr.cc (right): https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.cc#newcode33 util/mac/xattr.cc:33: do { Hmm… The man page says > When value is set to NULL, getxattr() returns current size of the named > attribute. This facility can be used to determine the size of a buffer > sufficiently large to hold the data currently associated with the attribute. Maybe that’s better than the loop altogether. https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.cc#newcode41 util/mac/xattr.cc:41: value->resize(value->size() * 2); …or if you keep this, just make sure there’s no way for it to logically overflow. https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.cc#newcod... util/mac/xattr.cc:115: int64_t encoded_value; #include <stdint.h>. https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.cc#newcod... util/mac/xattr.cc:120: if (!base::IsValueInRangeForNumericType<time_t, int64_t>(encoded_value)) { You can leave out the int64_t template parameters on this and the saturated_cast, because it’s implied by the type of encoded_value. Using the implicit value means that if the type changes, there are fewer things to clean up. For the same reason, you could use decltype(*value) instead of time_t as the template parameter. https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.h File util/mac/xattr.h (right): https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.h#newcode47 util/mac/xattr.h:47: bool WriteXattr(const base::FilePath& file, You may want ClearXattr at some point also, but it’s fine for now if it hasn’t proven necessary yet.
https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr_test.cc File util/mac/xattr_test.cc (right): https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:39: base::StringPrintf("/tmp/com.googlecode.crashpad.test.xattr.%p", this)); You didn’t use O_EXCL, but I can see why you might not want to with this using %p/this. OK, so stick with O_TRUNC instead of O_EXCL for now, but at least make things a little less likely to collide by using %d/getpid().
https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.cc File util/mac/xattr.cc (right): https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.cc#newcode33 util/mac/xattr.cc:33: do { On 2014/12/30 19:19:15, Mark Mentovai wrote: > Hmm… > > The man page says > > > When value is set to NULL, getxattr() returns current size of the named > > attribute. This facility can be used to determine the size of a buffer > > sufficiently large to hold the data currently associated with the attribute. > > Maybe that’s better than the loop altogether. Oh, I missed that. Yes, that's better. https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.cc#newcod... util/mac/xattr.cc:115: int64_t encoded_value; On 2014/12/30 19:19:15, Mark Mentovai wrote: > #include <stdint.h>. Done. https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.cc#newcod... util/mac/xattr.cc:120: if (!base::IsValueInRangeForNumericType<time_t, int64_t>(encoded_value)) { On 2014/12/30 19:19:15, Mark Mentovai wrote: > You can leave out the int64_t template parameters on this and the > saturated_cast, because it’s implied by the type of encoded_value. Using the > implicit value means that if the type changes, there are fewer things to clean > up. > > For the same reason, you could use decltype(*value) instead of time_t as the > template parameter. Done. https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.h File util/mac/xattr.h (right): https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr.h#newcode47 util/mac/xattr.h:47: bool WriteXattr(const base::FilePath& file, On 2014/12/30 19:19:15, Mark Mentovai wrote: > You may want ClearXattr at some point also, but it’s fine for now if it hasn’t > proven necessary yet. Acknowledged. https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr_test.cc File util/mac/xattr_test.cc (right): https://codereview.chromium.org/791493009/diff/40001/util/mac/xattr_test.cc#n... util/mac/xattr_test.cc:39: base::StringPrintf("/tmp/com.googlecode.crashpad.test.xattr.%p", this)); On 2014/12/30 19:21:51, Mark Mentovai wrote: > You didn’t use O_EXCL, but I can see why you might not want to with this using > %p/this. > > OK, so stick with O_TRUNC instead of O_EXCL for now, but at least make things a > little less likely to collide by using %d/getpid(). Done.
LGTM https://codereview.chromium.org/791493009/diff/60001/util/mac/xattr.cc File util/mac/xattr.cc (right): https://codereview.chromium.org/791493009/diff/60001/util/mac/xattr.cc#newcode17 util/mac/xattr.cc:17: #include <errno.h> Lose this now. https://codereview.chromium.org/791493009/diff/60001/util/mac/xattr.cc#newcode95 util/mac/xattr.cc:95: return base::StringToInt(tmp, value); If this is false, you should LOG something to maintain the documented contract. base::StringToInt() doesn’t LOG anything on its own. https://codereview.chromium.org/791493009/diff/60001/util/mac/xattr.cc#newcod... util/mac/xattr.cc:116: return false; Same here.
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 8e98c9251a2bfa276e2943eaccbfe1faba955d16 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/791493009/diff/60001/util/mac/xattr.cc File util/mac/xattr.cc (right): https://codereview.chromium.org/791493009/diff/60001/util/mac/xattr.cc#newcode17 util/mac/xattr.cc:17: #include <errno.h> On 2014/12/30 22:36:59, Mark Mentovai wrote: > Lose this now. Done. https://codereview.chromium.org/791493009/diff/60001/util/mac/xattr.cc#newcode95 util/mac/xattr.cc:95: return base::StringToInt(tmp, value); On 2014/12/30 22:36:59, Mark Mentovai wrote: > If this is false, you should LOG something to maintain the documented contract. > base::StringToInt() doesn’t LOG anything on its own. Done. https://codereview.chromium.org/791493009/diff/60001/util/mac/xattr.cc#newcod... util/mac/xattr.cc:116: return false; On 2014/12/30 22:36:59, Mark Mentovai wrote: > Same here. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
