Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1060913003:
Add output of error code for file operations in serialization_utils.cc. (Closed)
|
Created:
5 years, 8 months ago by d.samantaray Modified:
5 years, 8 months ago CC:
ostap Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd output of error code for file operations in serialization_utils.cc.
Add error number (errno) to the logging of lock/unlock/open operations to improve log output verbosity.
BUG=467586
Please review.
Committed: https://crrev.com/3d79d785171cf4cb0cbdf934b4690e10d93853b4
Cr-Commit-Position: refs/heads/master@{#325596}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Addressing comments #Patch Set 3 : Removing extra line from AUTHORS file #
Total comments: 10
Patch Set 4 : Addressing comments #
Total comments: 2
Patch Set 5 : Adressing the comments #Messages
Total messages: 25 (7 generated)
d.samantaray@samsung.com changed reviewers: + bsimonnet@chromium.org
Please review.
On 2015/04/06 11:35:37, d.samantaray wrote: > Please review. I would suggest to replace description with this: "Add output of error code for file operations in serialization_utils.cc. Add error number (errno) to the logging of lock/unlock/open operations to improve log output verbosity."
sl.ostapenko@samsung.com changed reviewers: + sl.ostapenko@samsung.com
https://codereview.chromium.org/1060913003/diff/1/components/metrics/serializ... File components/metrics/serialization/serialization_utils.cc (right): https://codereview.chromium.org/1060913003/diff/1/components/metrics/serializ... components/metrics/serialization/serialization_utils.cc:125: DPLOG(ERROR) << filename << ": bad metrics file stat" << " : " << errno; Please, make message consistent - filename after message (as in other places), or filename before message (as it was originally).
I have addressed the comments please review.
On 2015/04/08 03:07:49, d.samantaray wrote: > I have addressed the comments please review. lgtm
The CQ bit was checked by d.samantaray@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1060913003/40001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2015/04/08 03:07:49, d.samantaray wrote: > I have addressed the comments please review. I think isherman would be right reviewer for this. He reviewed those files here: https://codereview.chromium.org/394093003
d.samantaray@samsung.com changed reviewers: + isherman@chromium.org - bsimonnet@chromium.org, sl.ostapenko@samsung.com
On 2015/04/09 15:47:57, ostap wrote: > On 2015/04/08 03:07:49, d.samantaray wrote: > > I have addressed the comments please review. > > I think isherman would be right reviewer for this. He reviewed those files here: > https://codereview.chromium.org/394093003 I have uploaded the patch and notified Isherman, please review.
https://codereview.chromium.org/1060913003/diff/40001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/1060913003/diff/40001/AUTHORS#newcode563 AUTHORS:563: Debashish Samantaray <d.samantaray@samsung.com> It looks like this file is meant to be sorted alphabetically. Could you please alphabetize this entry? https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... File components/metrics/serialization/serialization_utils.cc (right): https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:125: DPLOG(ERROR) << " bad metrics file stat: " << filename << " : " << errno; Why do you have a leading space at the start of each logged line? https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:125: DPLOG(ERROR) << " bad metrics file stat: " << filename << " : " << errno; To the best of my knowledge, DPLOG already appends the errno. Why are you adding it again? https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:177: DLOG(ERROR) << "error openning the file: " << filename << " : " << errno; Rather than adding errno, can you use DPLOG? (Applies throughout.) https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:193: DLOG(ERROR) << "cannot write message: too long" << " : " << errno; Did you mean to include the filename here, as well as in the two log statements below?
FYI, just adding me to the list of reviewers does not notify me that you want me to review. Please send a message asking for a review each time you are ready for the next round of review, and each time that you add a new reviewer.
Dear Sherman, I have incorporated the changes. Please review. https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... File components/metrics/serialization/serialization_utils.cc (right): https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:125: DPLOG(ERROR) << " bad metrics file stat: " << filename << " : " << errno; On 2015/04/15 22:50:38, Ilya Sherman wrote: > Why do you have a leading space at the start of each logged line? Done. https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:125: DPLOG(ERROR) << " bad metrics file stat: " << filename << " : " << errno; On 2015/04/15 22:50:38, Ilya Sherman wrote: > To the best of my knowledge, DPLOG already appends the errno. Why are you > adding it again? Done. https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:125: DPLOG(ERROR) << " bad metrics file stat: " << filename << " : " << errno; On 2015/04/15 22:50:38, Ilya Sherman wrote: > Why do you have a leading space at the start of each logged line? Done. https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:177: DLOG(ERROR) << "error openning the file: " << filename << " : " << errno; On 2015/04/15 22:50:38, Ilya Sherman wrote: > Rather than adding errno, can you use DPLOG? (Applies throughout.) Done. https://codereview.chromium.org/1060913003/diff/40001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:193: DLOG(ERROR) << "cannot write message: too long" << " : " << errno; On 2015/04/15 22:50:38, Ilya Sherman wrote: > Did you mean to include the filename here, as well as in the two log statements > below? Done.
Thanks. Nearly there: https://codereview.chromium.org/1060913003/diff/60001/components/metrics/seri... File components/metrics/serialization/serialization_utils.cc (right): https://codereview.chromium.org/1060913003/diff/60001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:125: DPLOG(ERROR) << "bad metrics file stat:" << filename; You should keep the space *after* the colon, though, to separate the filename from the rest of the text.
Dear Sherman, I have made the changes. Please review. Thanks https://codereview.chromium.org/1060913003/diff/60001/components/metrics/seri... File components/metrics/serialization/serialization_utils.cc (right): https://codereview.chromium.org/1060913003/diff/60001/components/metrics/seri... components/metrics/serialization/serialization_utils.cc:125: DPLOG(ERROR) << "bad metrics file stat:" << filename; On 2015/04/16 21:50:49, Ilya Sherman wrote: > You should keep the space *after* the colon, though, to separate the filename > from the rest of the text. Done.
LGTM
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsimonnet@chromium.org Link to the patchset: https://codereview.chromium.org/1060913003/#ps80001 (title: "Adressing the comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1060913003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3d79d785171cf4cb0cbdf934b4690e10d93853b4 Cr-Commit-Position: refs/heads/master@{#325596} |
