Created attachment 36984 [details] [review]
The 'details' splash plugin (which Plymouth will fall back to in various situations, e.g. on a serial console) does not have a display_message function, and so any message display requests will be silently ignored.
On Ubuntu, messages sometimes include instructions on what key to press to continue boot (e.g. after filesystem mount failure); using the details splash plugin, these instructions are never displayed and the boot process simply appears to hang.
I attach a patch which adds support for message display to the 'details' plugin.
What is the keys: stuff about?
Ubuntu prepends "keys:" to messages which prompt the user with a list of possible keystrokes required to continue, so the graphical themes can display the message differently. The "keys:" tag itself is not intended to be displayed. I copied this behaviour from one of the Ubuntu themes.
If this behaviour is nonstandard, feel free to drop this part of the patch; Ubuntu can presumably re-add it locally.
Scott pointed out that this code should probably use 'sizeof ("keys:") - 1' rather than 'strlen (keys)'.
So one bit I'm a little worried about is this part:
+ if (plugin->state != PLY_BOOT_SPLASH_DISPLAY_NORMAL)
+ display_normal (plugin);
That suggests we're going to move the details plugin out of "ask the user for a password" mode if we decide to print a message, even if the user hasn't answered the question yet.
In practice, this may work for the details plugin since it doesn't do a lot when switching modes (not sure), but I think it doesn't read well.
It looks to me as though it would work, but be confusing; you'd have a password prompt some distance up the screen with messages below it, and it wouldn't be obvious that the system was waiting for password entry. (Not to mention that, in Ubuntu's case, the message can be from mountall indicating that you're supposed to press one of certain keys to respond to fsck failures and the like, and plymouth won't pull keystroke triggers until the password entry is finished anyway!)
So, would the best approach be to queue up the messages until display_normal is called, and then display them all at once?
I think that's reasonable. Any time a message comes while the user is being asked for a password, hold off until the user answers before showing the message.
Note, we already keep a list of messages in main.c. Maybe we just need to "replay the list".
It does suggest we need a way to drain the list, too, but that's probably something that can be handled separately as demand arises.
Created attachment 38522 [details] [review]
queue messages if necessary
This implements a local queue in details, pending work on a longer-term solution with decent lifetime support for messages (as discussed on IRC).
Created attachment 38524 [details] [review]
this time without the special-case keys: hack
One issue we may hit is distros may have been relying on details not displaying a message before. E.g., if they were doing a message
"Press escape to go to details mode"
It would look weird to see that message if the user is already in details mode. We may have to add some way to provide context to messages if that ends up being an issue.
So i'm tempted to revert this (see below conversation about a system update utility that's using plymouth). We could add a --mode argument to show-message and complicate the plugins more, but probably easier to just fix the caller in comment 0 to write to /dev/console instead?
<halfline> plymouth doesn't show the journal on escape
[15:58:58] <halfline> it just shows /dev/console
[15:59:05] <wwoods> halfline: ideally, they'd see the stdout (i.e. we'd be using journal+console) and not the show-message calls
[15:59:28] <wwoods> right now IIUC they get the show-message calls and nothing else
[15:59:51] <halfline> well if that's the behavior you want, we need to fix plymouth
[16:00:18] <halfline> or you'll end up with duplicated messages
[16:00:44] <wwoods> heh. well, yeah, but I can't dictate what other people are doing with plymouth, so I don't know if that behavior would mess up other stuff
[16:01:01] <halfline> meh
[16:01:19] <halfline> can't be afraid to make fixes
[16:01:26] <wwoods> still, the behavior *I* would expect would be that hitting Esc shows you whatever was on the console, and that display-message is only shown on the graphical splash(es)
[16:01:49] <wwoods> halfline: heh - well gosh, if the maintainer(s) think that's a good idea, I'm all for it
[16:03:04] <wwoods> halfline: looks like your second patch would implement that behavior - i.e. normal display-message messages are ignored by the details view?
[16:03:55] <halfline> yea i think so
[16:05:40] <wwoods> heh if there are types other than PLY_BOOT_SPLASH_DISPLAY_NORMAL, the user could specify, like, "display-message --text="$text" --show-on-all" or something, and there'd be a different message type which *would* get displayed in the details view
[16:06:18] <wwoods> but I don't know if that's a use case anyone cares about
[16:06:27] <halfline> actually that patch isn't quite right anyway, if the mode isn't display_normal we queue the messages until the next time the mode is display_normal
[16:06:46] <halfline> see also https://bugs.freedesktop.org/show_bug.cgi?id=29035
[16:07:17] <wwoods> heh, I figured that this would be one of those bugs where there are arguments for both behaviors
[16:07:31] <halfline> my comment 9 is pretty awesome
[16:07:53] <wwoods> prescient!
[16:09:58] <halfline> nothing is ever simple
[16:15:56] <halfline> i'm kind of tempted to just revert the patch and tell them to send their message to /dev/console too
[16:17:33] <wwoods> I mean... it kind of makes sense that the "details" view just shows what was printed to the console, and "display-message" messages are out-of-band from the console
-- GitLab Migration Automatic Message --
This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.
You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/plymouth/plymouth/issues/23.