New issue
Advanced search Search tips

Issue 611925 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocked on:
issue 333921

Blocking:
issue 722143



Sign in to add a comment

/etc/init/pre-startup.conf: drop/relocate old udev static logic

Reported by jrbarnette@chromium.org, May 14 2016

Issue description

The pre-startup Upstart job contains the following lines:
  # Create static device nodes from modules.devname.
  static_node_tool || logger -t "$UPSTART_JOB" "static_node_tool failed."

  # With udev-208, static kernel modules are no longer automatically handled,
  # so load tun and ppp_async devices manually.
  modprobe tun || logger -t "$UPSTART_JOB" "modprobe tun failed."
  modprobe ppp_async || logger -t "$UPSTART_JOB" "modprobe ppp_async failed."

  # Workaround for MIDI devices not being detected if none have been
  # connected before loading a Web MIDI page.
  # See  https://crbug.com/499817 .
  modprobe snd_seq_midi \
    || logger -t "$UPSTART_JOB" "modprobe snd_seq_midi failed."

There are some glaring problems with the code:
  * The code uses "logger".  The "pre-startup" job runs before
    rsyslogd starts, so output written at this stage goes to the
    bit bucket.
  * The code puts 'modprobe' commands on the critical path to boot.
    The pre-startup job runs before *everything*.  Blocking that job
    blocks mounting file systems and starting udev, neither of which
    depend on those modprobe commands.
  * The modules in question probably are board specific, and simply
    shouldn't be in a common upstart job like this in the first
    place.

 
Cc: bsimonnet@chromium.org gauravsh@chromium.org
Blockedon: 333921
Cc: -gauravsh@chromium.org -bsimonnet@chromium.org vapier@chromium.org
Status: Available (was: Unconfirmed)
Summary: /etc/init/pre-startup.conf: drop/relocate old udev static logic (was: suspicious, evil code in /etc/init/pre-startup.conf)
this logic was a straight forward port due to the new udev version.  it was done to preserve exactish behavior between the two versions to stave off regressions.

that said, now that things are stable, we can see about walking back things a bit.
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/7e2dc316a4ee1647f568741b4545698a2bced727

commit 7e2dc316a4ee1647f568741b4545698a2bced727
Author: Mike Frysinger <vapier@chromium.org>
Date: Sun Oct 09 21:38:27 2016

chromeos-init: install new static-nodes script

BUG= chromium:611925 
TEST=booted a system and these nodes still exist
CQ-DEPEND=CL:395747

Change-Id: I334400ccfbe2c344f894dfd0a69bf56019e5a182
Reviewed-on: https://chromium-review.googlesource.com/395927
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/7e2dc316a4ee1647f568741b4545698a2bced727/chromeos-base/chromeos-init/chromeos-init-9999.ebuild

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 10 2016

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

commit a8045a45488820c4cfe89958511ea5ec73c122ec
Author: Mike Frysinger <vapier@chromium.org>
Date: Sun Oct 09 21:35:36 2016

init: move static node creation to later in the boot sequence

We don't need the static nodes to be available during the initial startup
phase, so move it to the boot-services phase.  The nodes aren't used by
the chromeos_startup code.

BUG= chromium:611925 
TEST=booted a system and these nodes still exist
CQ-DEPEND=CL:395927

Change-Id: I818a2d27dd14caf4d647e82ab3a077a92cf9f047
Reviewed-on: https://chromium-review.googlesource.com/395747
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/a8045a45488820c4cfe89958511ea5ec73c122ec/init/pre-startup.conf
[add] https://crrev.com/a8045a45488820c4cfe89958511ea5ec73c122ec/init/static-nodes.conf

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/94472295e95e024cb7876caf6b8ad561cd0ab4a4

commit 94472295e95e024cb7876caf6b8ad561cd0ab4a4
Author: Mike Frysinger <vapier@chromium.org>
Date: Sun Oct 09 21:06:29 2016

chromeos-init: install new midi script

The pre-startup code blocks all boot logic and runs super early.
Move the midi code out into its own init script since it isn't
needed this early to slow things down.  Instead run in parallel
with other startup scripts that block the UI (Chrome).  It also
lets us install this workaround only on older kernels.

BUG= chromium:611925 
BUG= chromium:499817 
TEST=precq passes
CQ-DEPEND=CL:395986

Change-Id: I12dcb4fee123a32b4e007a86c254c8367d8f7ad2
Reviewed-on: https://chromium-review.googlesource.com/395969
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Richard Barnette <jrbarnette@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Adam Goode <agoode@google.com>

[modify] https://crrev.com/94472295e95e024cb7876caf6b8ad561cd0ab4a4/chromeos-base/chromeos-init/chromeos-init-9999.ebuild

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 12 2016

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

commit 5267ae57ac014e587d71b986848a7029a3423338
Author: Mike Frysinger <vapier@chromium.org>
Date: Sun Oct 09 21:03:08 2016

init: move midi workaround to later in boot

The pre-startup code blocks all boot logic and runs super early.
Move the midi code out into its own init script since it isn't
needed this early to slow things down.  Instead run in parallel
with other startup scripts that block the UI (Chrome).  It also
lets us install this workaround only on older kernels.

BUG= chromium:611925 
BUG= chromium:499817 
TEST=precq passes
CQ-DEPEND=CL:395969

Change-Id: I24f77b32d0b4ab99ec706f8f75276447f891c9f0
Reviewed-on: https://chromium-review.googlesource.com/395986
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Richard Barnette <jrbarnette@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Adam Goode <agoode@google.com>

[modify] https://crrev.com/5267ae57ac014e587d71b986848a7029a3423338/init/pre-startup.conf
[add] https://crrev.com/5267ae57ac014e587d71b986848a7029a3423338/init/workaround-init/midi-workaround.conf

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 18 2016

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

commit 36dfccd8b7924a4e58f8d8c213cf1878169fcc12
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Oct 14 03:46:01 2016

init: pre-startup: drop tun/ppp_async module loading

Since we create static nodes now (via static-nodes.conf), these modules
are loaded on demand when tools try to use those device nodes.  Drop the
manual loading during the critical boot paths since they aren't needed.

BUG= chromium:611925 
TEST=booted a system, saw tun module not loaded, ran tunctl, saw tap0 created, and tun module loaded

Change-Id: Ib8acf4633591872b5e1de36ddbd907ae2f438375
Reviewed-on: https://chromium-review.googlesource.com/399718
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/36dfccd8b7924a4e58f8d8c213cf1878169fcc12/init/pre-startup.conf

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 18 2016

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

commit 36dfccd8b7924a4e58f8d8c213cf1878169fcc12
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Oct 14 03:46:01 2016

init: pre-startup: drop tun/ppp_async module loading

Since we create static nodes now (via static-nodes.conf), these modules
are loaded on demand when tools try to use those device nodes.  Drop the
manual loading during the critical boot paths since they aren't needed.

BUG= chromium:611925 
TEST=booted a system, saw tun module not loaded, ran tunctl, saw tap0 created, and tun module loaded

Change-Id: Ib8acf4633591872b5e1de36ddbd907ae2f438375
Reviewed-on: https://chromium-review.googlesource.com/399718
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/36dfccd8b7924a4e58f8d8c213cf1878169fcc12/init/pre-startup.conf

Comment 9 by vapier@chromium.org, Oct 18 2016

Owner: vapier@chromium.org
Status: Fixed (was: Available)
cleaned up!

Comment 10 by dchan@google.com, Jan 21 2017

Labels: VerifyIn-57

Comment 11 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 12 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 13 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Blocking: 722143
for posterity, some of this rework broke static dev node permissions.  but that was resolved in  issue 722143  and these CLs:
  https://chromium-review.googlesource.com/527486
  https://chromium-review.googlesource.com/527463

the new code has comments too so hopefully it shouldn't regress :).  and we have peeps looking at autotests.
Labels: VerifyIn-61
Status: Verified (was: Fixed)
Verified on build 9824.0.0

Sign in to add a comment