Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26513005:
Report errors from ChromiumEnv::GetChildren in Posix. (Closed)
|
Created:
7 years, 2 months ago by dgrogan Modified:
7 years, 2 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReport errors from ChromiumEnv::GetChildren in Posix.
Win forthcoming.
BUG=308101
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229622
Patch Set 1 #Patch Set 2 : reupload #Patch Set 3 : add RecordOSError #Patch Set 4 : change to pfe #Patch Set 5 : added todo #Patch Set 6 : Split functions out from GetChildren #Patch Set 7 : return right type on windows #Patch Set 8 : fix compile on windows #
Total comments: 17
Patch Set 9 : respond to comments #Patch Set 10 : add BASE_EXPORT to ErrnoToPlatformFileError #
Total comments: 2
Patch Set 11 : add comment to PlatformFileError #
Total comments: 1
Messages
Total messages: 23 (0 generated)
Josh, could you review this?
I don't like pulling more platform specific code into env.
Could this be done by adding an PlatformFileError field to FileEnumerator and
either having Next() optionally take a PlatformFileError* or adding an
SawError() accessor? That may be messy since the POSIX and Windows APIs are
differently stateful.
If that's not feasible, I'd prefer there to be two separate implementation that
use FilePath, and could be relocated to base, e.g.
#if defined(OS_WIN)
PlatformFileError GetDirectoryEntries(const FilePath& dir, std::vector<FilePath>
entries) {
... FindFirstFile / FindNextFile / FindClose ...
}
#else
PlatformFileError GetDirectoryEntries(const FilePath& dir, std::vector<FilePath>
entries) {
... opendir/ readdir / closedir ...
}
#endif
... and then see about adding that to base's file_util
> PlatformFileError GetDirectoryEntries(const FilePath& dir, std::vector<FilePath> And yeah, that would require string<->FilePath conversions in the caller, but I think it would be a better separation of the directory enumeration from the env logic.
I think this is ready for another look.
Overall LGTM with nits/suggestions https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:674: #if defined(OS_WIN) Anonymous namespace around this. https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:675: base::PlatformFileError GetDirectoryEntries( And mark this static. (Examples in base seem to do both.) https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:679: base::FileEnumerator iter(dir_filepath, false, base::FileEnumerator::FILES); This is filtering out directories whereas the new POSIX version isn't. Although that difference is temporary until this is reimplemented, maybe a TODO about it? https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:688: base::PlatformFileError GetDirectoryEntries( Mark static too. https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:691: base::PlatformFileError return_value = base::PLATFORM_FILE_OK; return_value is unused https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:692: const std::string dir_string = dir_filepath.AsUTF8Unsafe(); Why not FilePathToString here? https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:699: int readdir_return; Name this readdir_result instead? https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:700: while ((readdir_return = readdir_r(dir, &dent_buf, &dent)) == 0 && dent) { Nit: No need for {}
https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:674: #if defined(OS_WIN) On 2013/10/17 18:32:39, jsbell wrote: > Anonymous namespace around this. Done. https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:675: base::PlatformFileError GetDirectoryEntries( On 2013/10/17 18:32:39, jsbell wrote: > And mark this static. (Examples in base seem to do both.) Done. Though I suspect the instances that of this in base are oversights. https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:679: base::FileEnumerator iter(dir_filepath, false, base::FileEnumerator::FILES); On 2013/10/17 18:32:39, jsbell wrote: > This is filtering out directories whereas the new POSIX version isn't. Although > that difference is temporary until this is reimplemented, maybe a TODO about it? Added comment. https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:688: base::PlatformFileError GetDirectoryEntries( On 2013/10/17 18:32:39, jsbell wrote: > Mark static too. Done. https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:691: base::PlatformFileError return_value = base::PLATFORM_FILE_OK; On 2013/10/17 18:32:39, jsbell wrote: > return_value is unused Done. https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:692: const std::string dir_string = dir_filepath.AsUTF8Unsafe(); On 2013/10/17 18:32:39, jsbell wrote: > Why not FilePathToString here? No good reason, switched to FilePathToString. They should be equivalent right? https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:699: int readdir_return; On 2013/10/17 18:32:39, jsbell wrote: > Name this readdir_result instead? Done. https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:700: while ((readdir_return = readdir_r(dir, &dent_buf, &dent)) == 0 && dent) { On 2013/10/17 18:32:39, jsbell wrote: > Nit: No need for {} Done.
isherman@, could you review this?
https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/26513005/diff/42001/third_party/leveldatabase... third_party/leveldatabase/env_chromium.cc:692: const std::string dir_string = dir_filepath.AsUTF8Unsafe(); On 2013/10/17 20:22:36, dgrogan wrote: > On 2013/10/17 18:32:39, jsbell wrote: > > Why not FilePathToString here? > > No good reason, switched to FilePathToString. They should be equivalent right? On non-Mac,non-CrOS, AsUTF8Unsafe() actually does a locale->UTF32->UTF8 conversion. We're not expecting non-ASCII filenames, but good to be consistent.
histograms.xml lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgrogan@chromium.org/26513005/47001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
thakis@, could you review as an owner of base?
https://codereview.chromium.org/26513005/diff/113001/third_party/leveldatabas... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/26513005/diff/113001/third_party/leveldatabas... third_party/leveldatabase/env_chromium.cc:720: RecordOSError(kGetChildren, error); Does this mean that PlatformFileError needs to have stable values now? If so, I'm not sure I like this. Could you convert this to some enum defined in this file instead, with a comment saying "this is used in a histogram, only add values"?
https://codereview.chromium.org/26513005/diff/113001/third_party/leveldatabas... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/26513005/diff/113001/third_party/leveldatabas... third_party/leveldatabase/env_chromium.cc:720: RecordOSError(kGetChildren, error); On 2013/10/18 02:28:30, Nico wrote: > Does this mean that PlatformFileError needs to have stable values now? If so, > I'm not sure I like this. Could you convert this to some enum defined in this > file instead, with a comment saying "this is used in a histogram, only add > values"? That ship may have already sailed. It's not just us who have been recording PlatformFileError in histograms, but the disk cache has been also. What's the reservation about PlatformFileError having stable values?
On 2013/10/18 20:52:58, dgrogan wrote: > https://codereview.chromium.org/26513005/diff/113001/third_party/leveldatabas... > File third_party/leveldatabase/env_chromium.cc (right): > > https://codereview.chromium.org/26513005/diff/113001/third_party/leveldatabas... > third_party/leveldatabase/env_chromium.cc:720: RecordOSError(kGetChildren, > error); > On 2013/10/18 02:28:30, Nico wrote: > > Does this mean that PlatformFileError needs to have stable values now? If so, > > I'm not sure I like this. Could you convert this to some enum defined in this > > file instead, with a comment saying "this is used in a histogram, only add > > values"? > > That ship may have already sailed. It's not just us who have been recording > PlatformFileError in histograms, but the disk cache has been also. :-/ > What's the reservation about PlatformFileError having stable values? Mostly, we want to be able to change base without breaking random stuff far away. Do you actually need all the granularity of PlatformFileError or is something more clumped more useful for you?
On 2013/10/18 20:58:05, Nico wrote: > On 2013/10/18 20:52:58, dgrogan wrote: > > > https://codereview.chromium.org/26513005/diff/113001/third_party/leveldatabas... > > File third_party/leveldatabase/env_chromium.cc (right): > > > > > https://codereview.chromium.org/26513005/diff/113001/third_party/leveldatabas... > > third_party/leveldatabase/env_chromium.cc:720: RecordOSError(kGetChildren, > > error); > > On 2013/10/18 02:28:30, Nico wrote: > > > Does this mean that PlatformFileError needs to have stable values now? If > so, > > > I'm not sure I like this. Could you convert this to some enum defined in > this > > > file instead, with a comment saying "this is used in a histogram, only add > > > values"? > > > > That ship may have already sailed. It's not just us who have been recording > > PlatformFileError in histograms, but the disk cache has been also. > > :-/ > > > What's the reservation about PlatformFileError having stable values? > > Mostly, we want to be able to change base without breaking random stuff far > away. > > Do you actually need all the granularity of PlatformFileError or is something > more clumped more useful for you? It's hard to say from the ex ante perspective. We've been recording errors at this granularity (and even finer granularity, in the form of "errno"s) for a while and some haven't turned out to be too helpful. But some of it has been very helpful, like finding that a leading cause of error on CrOS has been file descriptor exhaustion. Given that PlatformFileErrors have already been recorded by multiple components, what do you think of adding a comment to where the enum is defined, mentioning that fields can be added (as already noted there) but that the order can only be changed if no one is recording them anymore? I'd add that to this CL, obviously.
On Fri, Oct 18, 2013 at 2:33 PM, <dgrogan@chromium.org> wrote: > On 2013/10/18 20:58:05, Nico wrote: > >> On 2013/10/18 20:52:58, dgrogan wrote: >> > >> > > https://codereview.chromium.**org/26513005/diff/113001/** > third_party/leveldatabase/env_**chromium.cc<https://codereview.chromium.org/26513005/diff/113001/third_party/leveldatabase/env_chromium.cc> > >> > File third_party/leveldatabase/env_**chromium.cc (right): >> > >> > >> > > https://codereview.chromium.**org/26513005/diff/113001/** > third_party/leveldatabase/env_**chromium.cc#newcode720<https://codereview.chromium.org/26513005/diff/113001/third_party/leveldatabase/env_chromium.cc#newcode720> > >> > third_party/leveldatabase/env_**chromium.cc:720: >> RecordOSError(kGetChildren, >> > error); >> > On 2013/10/18 02:28:30, Nico wrote: >> > > Does this mean that PlatformFileError needs to have stable values >> now? If >> so, >> > > I'm not sure I like this. Could you convert this to some enum defined >> in >> this >> > > file instead, with a comment saying "this is used in a histogram, >> only add >> > > values"? >> > >> > That ship may have already sailed. It's not just us who have been >> recording >> > PlatformFileError in histograms, but the disk cache has been also. >> > > :-/ >> > > > What's the reservation about PlatformFileError having stable values? >> > > Mostly, we want to be able to change base without breaking random stuff >> far >> away. >> > > Do you actually need all the granularity of PlatformFileError or is >> something >> more clumped more useful for you? >> > > It's hard to say from the ex ante perspective. We've been recording errors > at > this granularity (and even finer granularity, in the form of "errno"s) for > a > while and some haven't turned out to be too helpful. But some of it has > been > very helpful, like finding that a leading cause of error on CrOS has been > file > descriptor exhaustion. > > Given that PlatformFileErrors have already been recorded by multiple > components, > what do you think of adding a comment to where the enum is defined, > mentioning > that fields can be added (as already noted there) but that the order can > only be > changed if no one is recording them anymore? I'd add that to this CL, > obviously. > Sounds fine. I'd phrase it like "This is recorded in multiple histograms, so if you need to change the order you first need to move all clients off of this" or similar (in slightly more formal language I) > > https://codereview.chromium.**org/26513005/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/26513005/diff/215001/base/platform_file.h File base/platform_file.h (right): https://codereview.chromium.org/26513005/diff/215001/base/platform_file.h#new... base/platform_file.h:60: // have been moved to a different enum. thakis@, please take a look
base/ lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgrogan@chromium.org/26513005/215001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgrogan@chromium.org/26513005/215001
Message was sent while issue was closed.
Change committed as 229622 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
