|
|
Descriptionrlz: Silence spam from RecursiveCrossProcessLock
Change perror() to VPLOG() to avoid spamming the console with these
errors (they reliably occur on the login screen).
BUG=463306
TEST=build, boot, & login to pixel
Committed: https://crrev.com/ce89b915d34a6bbe645da530b61bc3461e49f70f
Cr-Commit-Position: refs/heads/master@{#326621}
Patch Set 1 #Patch Set 2 : oops.. add .value() #Messages
Total messages: 18 (6 generated)
spang@chromium.org changed reviewers: + thakis@chromium.org
thakis@chromium.org changed reviewers: + ivankr@chromium.org
Maybe we shouldn't call this code while on the login screen then? +ivankr who did the rlz/cros stuff iirc
On 2015/04/22 17:05:49, Nico (OOO sick) wrote: > Maybe we shouldn't call this code while on the login screen then? > > +ivankr who did the rlz/cros stuff iirc It probably shouldn't. I don't remember well how it works and I've left ChromeOS team long ago, Nikita should know better who's in charge of RLZ now. Do you have a stack trace of how this gets called on login screen?
On 2015/04/22 17:59:11, Ivan Korotkov wrote: > On 2015/04/22 17:05:49, Nico (OOO sick) wrote: > > Maybe we shouldn't call this code while on the login screen then? > > > > +ivankr who did the rlz/cros stuff iirc > > It probably shouldn't. I don't remember well how it works and I've left ChromeOS > team long ago, Nikita should know better who's in charge of RLZ now. > Do you have a stack trace of how this gets called on login screen? https://code.google.com/p/chromium/issues/detail?id=463306#c6
On 2015/04/22 17:05:49, Nico (OOO sick) wrote: > Maybe we shouldn't call this code while on the login screen then? > > +ivankr who did the rlz/cros stuff iirc I agree, but regardless a mutex implementation must not log to stderr. Can we discuss that part please? Do you think VPLOG(1) is too quiet? PLOG(ERROR) is probably OK if we fix the underlying problem.
On 2015/04/23 17:47:32, spang wrote: > On 2015/04/22 17:05:49, Nico (OOO sick) wrote: > > Maybe we shouldn't call this code while on the login screen then? > > > > +ivankr who did the rlz/cros stuff iirc > > I agree, but regardless a mutex implementation must not log to stderr. But this should never fail in practice, from what I understand. I haven't heard reports of this ever failing on OS X. This suggests that this code is being called at a time it shouldn't be called on OS X, and this CL is just fixing symptoms. > > Can we discuss that part please? > > Do you think VPLOG(1) is too quiet? > > PLOG(ERROR) is probably OK if we fix the underlying problem.
On 2015/04/23 17:52:40, Nico (OOO sick) wrote: > On 2015/04/23 17:47:32, spang wrote: > > On 2015/04/22 17:05:49, Nico (OOO sick) wrote: > > > Maybe we shouldn't call this code while on the login screen then? > > > > > > +ivankr who did the rlz/cros stuff iirc > > > > I agree, but regardless a mutex implementation must not log to stderr. > > But this should never fail in practice, from what I understand. I haven't heard > reports of this ever failing on OS X. This suggests that this code is being > called at a time it shouldn't be called on OS X, and this CL is just fixing > symptoms. Again, I acknowledge we don't think it should fail, but the logging here is also broken. I'm asking for a code review on the logging change only. Calling perror("open") to log is unacceptable because it doesn't provide enough context to find the call site. PLOG(ERROR) will add the source path & line number. > > > > > Can we discuss that part please? > > > > Do you think VPLOG(1) is too quiet? > > > > PLOG(ERROR) is probably OK if we fix the underlying problem.
lgtm Probably not worth the time arguing about this :-)
The CQ bit was checked by spang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096203003/1
The CQ bit was unchecked by spang@chromium.org
The CQ bit was checked by spang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1096203003/#ps20001 (title: "oops.. add .value()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096203003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ce89b915d34a6bbe645da530b61bc3461e49f70f Cr-Commit-Position: refs/heads/master@{#326621} |