Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
248653002:
Linux Breakpad: Keep reading from the crash upload progress until a complete crash id has been rece… (Closed)
|
Created:
6 years, 8 months ago by Lei Zhang Modified:
6 years, 8 months ago CC:
chromium-reviews Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionLinux Breakpad: Keep reading from the crash upload progress until a complete crash id has been received.
BUG=363660
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266286
Patch Set 1 #Patch Set 2 : handle cros #
Total comments: 2
Patch Set 3 : #Patch Set 4 : refactor, fix stupid mistakes, fix lint errors #
Total comments: 9
Patch Set 5 : address comments #Patch Set 6 : O_CLOEXEC #Messages
Total messages: 19 (0 generated)
jochen: Please review vapier/achaulk: Is this going to work well with crash_reporter? Does crash_reporter send back a 16 digit crash id? I forget...
lgtm
No, crash_reporter doesn't do the upload immediately, so nothing gets returned
On 2014/04/23 13:46:04, achaulk wrote: > No, crash_reporter doesn't do the upload immediately, so nothing gets returned Maybe we should make crash_reporter output something, so the browser process knows its done.
the only thing this code does is update the crash log, but in the system crash reporter situation, we don't want Chrome doing that i don't think we could have it write back like 16 NUL chars and you could check for that merely as a signal that the system crash reporter has successfully accepted the report ...
On 2014/04/23 21:18:07, vapier wrote: > the only thing this code does is update the crash log, but in the system crash > reporter situation, we don't want Chrome doing that i don't think > > we could have it write back like 16 NUL chars and you could check for that > merely as a signal that the system crash reporter has successfully accepted the > report ... How about we make crash_reporter reply with "cros_cr_finished", and we'll conditionally check for that is OS_CHROMEOS is defined?
if we used "_sys_cr_finished", it'd be generic and not dependent upon CrOS :)
On 2014/04/23 21:30:55, vapier wrote: > if we used "_sys_cr_finished", it'd be generic and not dependent upon CrOS :) We are not trying to make it generic though. On Linux, the 16 char reply is the crash report id. "_sys_cr_finished" is fine though.
On 2014/04/23 21:34:32, Lei Zhang wrote: > On 2014/04/23 21:30:55, vapier wrote: > > if we used "_sys_cr_finished", it'd be generic and not dependent upon CrOS :) > > We are not trying to make it generic though. On Linux, the 16 char reply is the > crash report id. "_sys_cr_finished" is fine though. See patch set 2. Want me to do the crash_reporter change?
well, i think the fewer #ifdefs, the better. at least in the crash UI, i've been trying to make the concept of "system crash reporter" kind of generic. i won't block on it though. if you want to update the CrOS crash-reporter, sgtm :) https://codereview.chromium.org/248653002/diff/20001/components/breakpad/app/... File components/breakpad/app/breakpad_linux.cc (right): https://codereview.chromium.org/248653002/diff/20001/components/breakpad/app/... components/breakpad/app/breakpad_linux.cc:1030: char c = buf[i]; i think you want: for (size_t i = 0; i < buflen; ++i) if (!isxdigit(buf[i])) return false; return true; is there not a libbase func to do this too ?
I fixed things a bit and did some refactoring in patch set 4. HandleCrashDump() is *only* 388 lines long now! https://codereview.chromium.org/248653002/diff/20001/components/breakpad/app/... File components/breakpad/app/breakpad_linux.cc (right): https://codereview.chromium.org/248653002/diff/20001/components/breakpad/app/... components/breakpad/app/breakpad_linux.cc:1030: char c = buf[i]; On 2014/04/23 22:03:21, vapier wrote: > i think you want: > > for (size_t i = 0; i < buflen; ++i) > if (!isxdigit(buf[i])) > return false; > return true; > > is there not a libbase func to do this too ? We can't call into libc in the middle of a crash. Note the numerous my_foo functions used in this file.
https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... File components/breakpad/app/breakpad_linux.cc (right): https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... components/breakpad/app/breakpad_linux.cc:1058: buf[bytes_to_read] = 0; // Always NULL terminate the buffer. you NUL terminate the buffer ;) https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... components/breakpad/app/breakpad_linux.cc:1072: if ((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) i was suggesting ctypes also to avoid case issues. but you can handle that with 0x20: bool my_isxdigit(char c) { return (c >= '0' && c <= '9') || ((c | 0x20) >= 'a' && (c | 0x20) <= 'f'); } https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... components/breakpad/app/breakpad_linux.cc:1091: WriteNewline(); why not embed the newline ? you do below ... https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... components/breakpad/app/breakpad_linux.cc:1113: const int kLogOpenFlags = O_CREAT | O_WRONLY | O_APPEND; in general, any reason to not use O_CLOEXEC ? in old kernels, it'll be silently ignored.
https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... File components/breakpad/app/breakpad_linux.cc (right): https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... components/breakpad/app/breakpad_linux.cc:1058: buf[bytes_to_read] = 0; // Always NULL terminate the buffer. On 2014/04/24 00:47:39, vapier wrote: > you NUL terminate the buffer ;) Done. https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... components/breakpad/app/breakpad_linux.cc:1072: if ((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) On 2014/04/24 00:47:39, vapier wrote: > i was suggesting ctypes also to avoid case issues. but you can handle that with > 0x20: > > bool my_isxdigit(char c) { > return (c >= '0' && c <= '9') || ((c | 0x20) >= 'a' && (c | 0x20) <= 'f'); > } Done. It's all lower case right now, but why not handle it? https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... components/breakpad/app/breakpad_linux.cc:1091: WriteNewline(); On 2014/04/24 00:47:39, vapier wrote: > why not embed the newline ? you do below ... WriteLog() below is a bit more complicated. There's a common path here. https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... components/breakpad/app/breakpad_linux.cc:1113: const int kLogOpenFlags = O_CREAT | O_WRONLY | O_APPEND; On 2014/04/24 00:47:39, vapier wrote: > in general, any reason to not use O_CLOEXEC ? > > in old kernels, it'll be silently ignored. We probably wrote this when it was still brand new. Does this matter here? This is in the helper process which never execs.
https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... File components/breakpad/app/breakpad_linux.cc (right): https://codereview.chromium.org/248653002/diff/110001/components/breakpad/app... components/breakpad/app/breakpad_linux.cc:1113: const int kLogOpenFlags = O_CREAT | O_WRONLY | O_APPEND; it frequently doesn't matter, but O_CLOEXEC is a defensive thing. we start off not expecting many scenarios, but code paths change over time :).
lgtm
O_CLOEXEC it is. As soon as I can actually build platform2, I'll go fix crash_reporter and then land this.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/248653002/150001
Message was sent while issue was closed.
Change committed as 266286 |
