New issue
Advanced search Search tips

Issue 874698 link

Starred by 0 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Feature

Blocked on:
issue 904728



Sign in to add a comment

crash-reporter: anomaly_collector: convert to C++, rewrite unit test

Project Member Reported by vapier@chromium.org, Aug 16

Issue description

this started off as a simple lexer/tool, but has grown too much.  we need to make most of the code C++ and reduce the .l file so it is only the lexing logic.  we should also be able to leverage %option c++ to make the lexer itself C++.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/e10b2294bd1a608e38f817a4e46fba1d6a21c204

commit e10b2294bd1a608e38f817a4e46fba1d6a21c204
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Aug 16 20:14:46 2018

crash: drop unused anomaly_collector_test.c

This hasn't been used in a long time.  The ebuild and gyp files just
refer to the anomaly_collector_test.sh helper directly now.

BUG=chromium:874698
TEST=precq passes

Change-Id: I157a8d2fa3b499b465e234408586e87fc0ff4cda
Reviewed-on: https://chromium-review.googlesource.com/1176639
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Satoru Takabayashi <satorux@google.com>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[delete] https://crrev.com/42c98eaccd9e3fa42a32f9c0c604cea82f9ae0de/crash-reporter/anomaly_collector_test.c

Comment 2 Deleted

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/cbd55f9da5d885740cb5b40fb4de2805869350b9

commit cbd55f9da5d885740cb5b40fb4de2805869350b9
Author: Mike Frysinger <vapier@chromium.org>
Date: Sat Sep 08 01:37:01 2018

crash: anomaly_collector: switch to stdin & temp files

Rather than use a named path in /run to communicate between
anomaly_collector and crash_reporter, switch to using stdin.
This simplifies the code a bit, especially testing-wise, and
avoids privileged paths.

BUG=chromium:874698
TEST=precq passes

Change-Id: Iaffd5ad9f4aa4000d4f211a4840effac59c3a6e5
Reviewed-on: https://chromium-review.googlesource.com/1176640
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Satoru Takabayashi <satorux@google.com>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/cbd55f9da5d885740cb5b40fb4de2805869350b9/crash-reporter/kernel_warning_collector.cc
[modify] https://crrev.com/cbd55f9da5d885740cb5b40fb4de2805869350b9/crash-reporter/README.md
[modify] https://crrev.com/cbd55f9da5d885740cb5b40fb4de2805869350b9/crash-reporter/anomaly_collector_test_reporter.sh
[modify] https://crrev.com/cbd55f9da5d885740cb5b40fb4de2805869350b9/crash-reporter/anomaly_collector.l
[modify] https://crrev.com/cbd55f9da5d885740cb5b40fb4de2805869350b9/crash-reporter/selinux_violation_collector.cc
[modify] https://crrev.com/cbd55f9da5d885740cb5b40fb4de2805869350b9/crash-reporter/service_failure_collector.cc
[modify] https://crrev.com/cbd55f9da5d885740cb5b40fb4de2805869350b9/crash-reporter/docs/security.md
[modify] https://crrev.com/cbd55f9da5d885740cb5b40fb4de2805869350b9/crash-reporter/docs/collectors.md

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/61664e4613fe8f05a0989f9180a1d5dfbeec2c50

commit 61664e4613fe8f05a0989f9180a1d5dfbeec2c50
Author: Mike Frysinger <vapier@chromium.org>
Date: Sat Sep 08 13:49:54 2018

crash: anomaly_collector: drop --fifo flag

Nothing uses this, and it's less effort to drop it than to try and
convert it to the new C++ form.  People can use the existing --test
flag anyways:
$ anomaly_collector --test <fifo

BUG=chromium:874698
TEST=precq passes

Change-Id: I854a8e0e3fbed1186faf1df54ef8d7ed5a1ae46e
Reviewed-on: https://chromium-review.googlesource.com/1188857
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>

[modify] https://crrev.com/61664e4613fe8f05a0989f9180a1d5dfbeec2c50/crash-reporter/anomaly_collector.l

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/c348da544894a4868edda1099d186cee97306884

commit c348da544894a4868edda1099d186cee97306884
Author: Mike Frysinger <vapier@chromium.org>
Date: Sun Sep 09 17:33:54 2018

crash: anomaly_collector: start converting to C++

This starts a new C++ file to hold the main entry point.  This way
we can start peeling away non-lexing logic from the .l file.

BUG=chromium:874698
TEST=precq passes

Change-Id: I6d3d1776dc38e6df08d923998dff9da2f3eef8ac
Reviewed-on: https://chromium-review.googlesource.com/1188858
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>

[add] https://crrev.com/c348da544894a4868edda1099d186cee97306884/crash-reporter/anomaly_collector.cc
[modify] https://crrev.com/c348da544894a4868edda1099d186cee97306884/crash-reporter/anomaly_collector.l
[modify] https://crrev.com/c348da544894a4868edda1099d186cee97306884/crash-reporter/crash-reporter.gyp
[add] https://crrev.com/c348da544894a4868edda1099d186cee97306884/crash-reporter/anomaly_collector.h

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/f53b8c16e4cf4a921a36d898bec98cbba77b78f1

commit f53b8c16e4cf4a921a36d898bec98cbba77b78f1
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Sep 14 01:58:34 2018

crash: anomaly_collector: move state dir setup to C++

BUG=chromium:874698
TEST=precq passes

Change-Id: I9bc9cae0d340a586af7a3eead315e0c71dbda697
Reviewed-on: https://chromium-review.googlesource.com/1214631
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>

[modify] https://crrev.com/f53b8c16e4cf4a921a36d898bec98cbba77b78f1/crash-reporter/anomaly_collector.cc
[modify] https://crrev.com/f53b8c16e4cf4a921a36d898bec98cbba77b78f1/crash-reporter/anomaly_collector.l

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/a2c7247ab166fa36cedec289b047e483876244f0

commit a2c7247ab166fa36cedec289b047e483876244f0
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Sep 14 01:58:34 2018

crash: anomaly_collector: replace custom Die() with err()

This simplifies the code a bit by using err() from err.h from the C lib.

BUG=chromium:874698
TEST=precq passes

Change-Id: Ib46f5307d2f9f2ff119f83a29660aeef5997499c
Reviewed-on: https://chromium-review.googlesource.com/1214632
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>

[modify] https://crrev.com/a2c7247ab166fa36cedec289b047e483876244f0/crash-reporter/anomaly_collector.l

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/1646b863efc0b48eb4563a93ce180eed1d5e11b6

commit 1646b863efc0b48eb4563a93ce180eed1d5e11b6
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Sep 14 01:58:35 2018

crash: anomaly_collector: improve filter logic a bit

Some of the sub-collectors are inconsistent in their handling of the
filter flag.  Push it down into the common AnomalyStart helper so we
can delete it from the individual helpers.

We also allow AnomalyEnd to run, but we use /bin/cat to dump the log
to stdout.

BUG=chromium:874698
TEST=precq passes

Change-Id: I4ff32659b5271b5f1973f177cefcc4f6d6652e8d
Reviewed-on: https://chromium-review.googlesource.com/1214633
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>

[modify] https://crrev.com/1646b863efc0b48eb4563a93ce180eed1d5e11b6/crash-reporter/anomaly_collector.l

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/ec2efa99eb86af9a12dd356e13178684c818d857

commit ec2efa99eb86af9a12dd356e13178684c818d857
Author: Mike Frysinger <vapier@chromium.org>
Date: Sat Sep 15 19:56:22 2018

crash: anomaly_collector: simplify test wrapper logic

Make anomaly_collector_test_reporter.sh handle the same flags as
crash_reporter so that we can push all that logic out of the C++
code.  This way anomaly_collector can pass the same set of flags
down regardless of testing state.

BUG=chromium:874698
TEST=precq passes

Change-Id: I8164d2ce93b8797eff5664019ec51b0b829517e9
Reviewed-on: https://chromium-review.googlesource.com/1214634
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>

[modify] https://crrev.com/ec2efa99eb86af9a12dd356e13178684c818d857/crash-reporter/anomaly_collector.cc
[modify] https://crrev.com/ec2efa99eb86af9a12dd356e13178684c818d857/crash-reporter/anomaly_collector.l
[modify] https://crrev.com/ec2efa99eb86af9a12dd356e13178684c818d857/crash-reporter/anomaly_collector_test_reporter.sh
[modify] https://crrev.com/ec2efa99eb86af9a12dd356e13178684c818d857/crash-reporter/anomaly_collector.h

Project Member

Comment 10 by bugdroid1@chromium.org, Sep 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/dea98c10047db8950a9a7de122ddd03b6e3ff83e

commit dea98c10047db8950a9a7de122ddd03b6e3ff83e
Author: Mike Frysinger <vapier@chromium.org>
Date: Sun Sep 16 19:03:32 2018

crash: anomaly_collector: mark all vars/funcs as static

None of these need to be exported, so mark them all static.  This
should make it much easier to keep track of unused definitions.

BUG=chromium:874698
TEST=precq passes

Change-Id: Ib887ef0d79f0cf5ca5837d2b114b4b5cef302a0b
Reviewed-on: https://chromium-review.googlesource.com/1214635
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/dea98c10047db8950a9a7de122ddd03b6e3ff83e/crash-reporter/anomaly_collector.l

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/5bb6ad4c07299b88eeb7d9a17b10cbc2d4c4ed27

commit 5bb6ad4c07299b88eeb7d9a17b10cbc2d4c4ed27
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Sep 28 02:45:16 2018

crash: anomaly-collector: run through minijail for namespaces

BUG=chromium:874698
TEST=precq passes

Change-Id: I5551d39d3a95713eefbca36a9162a6042b3365bf
Reviewed-on: https://chromium-review.googlesource.com/1214636
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/5bb6ad4c07299b88eeb7d9a17b10cbc2d4c4ed27/crash-reporter/init/anomaly-collector.conf

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/2ca91f91da7169e8f620d0ce518a416ca9dcd481

commit 2ca91f91da7169e8f620d0ce518a416ca9dcd481
Author: Mike Frysinger <vapier@chromium.org>
Date: Mon Oct 01 18:31:03 2018

crash: update anomaly_collector links

Lets drive people to the C++ file.

BUG=chromium:874698
TEST=precq passes

Change-Id: I766d5ec3628d5afcc8aeda549d5a7501a23298bd
Reviewed-on: https://chromium-review.googlesource.com/1229277
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>

[modify] https://crrev.com/2ca91f91da7169e8f620d0ce518a416ca9dcd481/crash-reporter/docs/security.md
[modify] https://crrev.com/2ca91f91da7169e8f620d0ce518a416ca9dcd481/crash-reporter/docs/collectors.md
[modify] https://crrev.com/2ca91f91da7169e8f620d0ce518a416ca9dcd481/crash-reporter/README.md

Summary: crash-reporter: anomaly_collector: convert to C++, rewrite unit test (was: crash-reporter: anomaly_collector: convert to C++)
Incidentally, the unit testing needs a complete rehaul.  In the state it is now it could be a good start for an autotest, but it's intolerably racy for a unit test.  This is mostly my fault.
i've landed all the CLs i had pending at this point

i think based on other work going on, we need to start a anomaly_collector_util.cc file and migrate content to that.  that way the anomaly_collector.cc and (to be written) anomaly_collector_test.cc can unittest everything directly.

might even want to pull some content out of anomaly_collector.cc to unittest where makes sense.
Blockedon: 904728

Sign in to add a comment