Review: Disapprove
I think there's some very unfortunate scope creep here. My previous revision
was addressing what would have been a regression for users of static /dev, due
to the incorrect overmounting with devtmpfs. This subsequent change doesn't
address any problems related to the /dev/pts case at all, instead adding checks
for additional devices that have never been the source of error reports.
+ if (system_check_file ("/dev/null", S_IFCHR, makedev (1, 3)) < 0)
+ needs_devtmpfs = 1;
+
+ if (system_check_file ("/dev/console", S_IFCHR, makedev (5, 1)) < 0)
+ needs_devtmpfs = 1;
+
+ if (system_check_file ("/dev/tty", S_IFCHR, makedev (5, 0)) < 0)
+ needs_devtmpfs = 1;
+
This is a completely unnecessary check. These three device nodes are
guaranteed to *always* be present at boot time. Either they're set up by the
initramfs, or they're required to be part of /dev on the root filesystem, or
the system must be configured for the kernel to automount devtmpfs at boot
time. It is therefore *wrong* for upstart to take action when these device
nodes are missing, as that means the system is badly broken and requiring admin
intervention, and papering over one of the symptoms will only make it harder
for the problem to be fixed at its source. This adds complexity where none is
needed.
+ if (system_check_file ("/dev/kmsg", S_IFCHR, makedev (1, 11)) < 0)
+ needs_devtmpfs = 1;
This is a device that could possibly be missing (it's not included in the "std"
set from MAKEDEV), but it also seems optional... if missing, you just don't get
messages logged, right?
I think it's borderline as to whether upstart should do anything to create
/dev/kmsg. If it's not present at boot time, chances are that it will be
present soon afterwards. On the other hand, if it's going to become "present
soon afterwards", it means we have a dynamic /dev, so there's probably no
*harm* in mounting /dev here. But I think it's still orthogonal to the problem
this branch was supposed to solve and should be treated separately.
So the only part of this that I would include is the fix of the mknod()
argument ordering. (Which, btw, you probably never noticed problems with when
testing because the node is created by the kernel at the same time as
/dev/null, so in practice it's always present by the time devtmpfs is mounted.)
--
https://code.launchpad.net/~jamesodhunt/upstart/bug-980917-reworked/+merge/118132
Your team Upstart Reviewers is subscribed to branch lp:upstart.
--
upstart-devel mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/upstart-devel
