Send plymouth mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
http://lists.freedesktop.org/mailman/listinfo/plymouth
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of plymouth digest..."
Today's Topics:
1. Re: Problem with redirecting output to boot terminal (Ray Strode)
2. Re: Plymouth/Upstart integration (Ray Strode)
----------------------------------------------------------------------
Message: 1
Date: Tue, 14 Dec 2010 15:07:26 -0500
From: Ray Strode <[email protected]>
Subject: Re: Problem with redirecting output to boot terminal
To: Andrey Borzenkov <[email protected]>
Cc: [email protected]
Message-ID:
<[email protected]>
Content-Type: text/plain; charset=UTF-8
Hi,
(sorry for the slow reply)
On Sun, Nov 28, 2010 at 2:22 PM, Andrey Borzenkov <[email protected]> wrote:
> It took me some time to understand why output from rc.sysinit was not visible.
>
> In initrd we start plymouthd and call "plymouth --show-splash". Now I
> do not have any splash to show (i.e. no "splash" on command line) but
> plymouthd still switches active console to boot terminal which in our
> case in tty7.
Even when "splash" is taken off the kernel command line, we still show
a splash called "details"
This splash basically just echos all data coming in and spews it back out.
It does do a little more if you have encrypted disks.
> Later rc.sysinit starts, checks whether splash is active and
> terminates plymouthd by calling "plymouth quit".
why do you quit plymouth that early?
One thing to keep in mind is, plymouth performs two primary functions:
1) boot splash
2) boot message logger
So even if you take "splash" off the kernel command line, it still
logs your boot.
Normally, you would run plymouth all the way until the end of boot up,
so that all boot messages get logged to /var/log/boot.log
> Only after next script starts anything appears on tty1.
>
> The question is - why would we switch to tty7 (or whatever is defined
> in configuration) when we know that no splash is going to be shown
> anyway? This splits boot output between several terminals and is
> really confusing.
Honestly, I think it's a better choice to run plymouth on tty1 and run
X on tty1. It makes the most sense looking at things from a "fresh"
perspective. It's only tty7 by historical accident, basically. I
recognize not everyone shares my view, though.
--Ray
------------------------------
Message: 2
Date: Tue, 14 Dec 2010 17:56:05 -0500
From: Ray Strode <[email protected]>
Subject: Re: Plymouth/Upstart integration
To: Colin Watson <[email protected]>
Cc: [email protected], [email protected]
Message-ID:
<[email protected]>
Content-Type: text/plain; charset=UTF-8
Hi,
(one the one month anniversary of your message...wheee! sorry about that)
On Sun, Nov 14, 2010 at 10:23 AM, Colin Watson <[email protected]> wrote:
> I've been working on solving the problem whereby users of Ubuntu server
> systems (in particular) get no indication that Upstart jobs are
> starting. ?Following suggestions from Scott James Remnant and Ray
> Strode, I've written a plymouth-upstart-bridge process which connects to
> Upstart's D-Bus interface and provides updates to Plymouth as events
> change state. ?Here's a preliminary patch for review.
>
> This patch depends on
> https://code.launchpad.net/~cjwatson/upstart/state-changed and
> https://code.launchpad.net/~cjwatson/upstart/goal-changed, which add a
> few more features to Upstart's D-Bus interface.
[...]
> Still, I think this is far enough along to ask for review. ?Comments?
So, I read through it a bit this afternoon. I have some comments
inline (there is some overlap with things you've already mentioned)
> +AC_ARG_ENABLE(upstart, AS_HELP_STRING([--enable-upstart],[listen for
> messages on the Upstart D-Bus
> interface]),enable_upstart=$enableval,enable_upstart=no)
I think i'd rather this was called
--enable-upstart-monitoring
or
--enable-upstart-integration
or something along those lines
> +if test x$enable_upstart = xyes; then
> + PKG_CHECK_MODULES(DBUS, [dbus-1])
> + AC_SUBST(DBUS_CFLAGS)
> + AC_SUBST(DBUS_LIBS)
> + AC_CHECK_HEADERS([ncursesw/term.h ncurses/term.h term.h], [break])
> + AC_CHECK_LIB([ncursesw], [initscr],
> + [CURSES_LIBS="$CURSES_LIBS -lncursesw"],
> + [AC_CHECK_LIB([ncurses], [initscr],
> + [CURSES_LIBS="$CURSES_LIBS -lncurses"],
> + [AC_CHECK_LIB([curses], [initscr],
> + [CURSES_LIBS="$CURSES_LIBS -lcurses"],
> + [AC_MSG_ERROR([no curses library found])])])])
> + AC_SUBST(CURSES_LIBS)
> +fi
Why do you need ncurses. If there's a good reason for it, then linking against
it in the bridge isn't a problem, of course. I will say there is primitive
terminal handling code in ply-terminal.[ch] and ply-text-display.[ch]. We
could potentially move part of that code out of libply-splash-core into
libply/.
Why aren't you using pkgconfig for ncurses though?
> --- a/src/client/Makefile.am
> +++ b/src/client/Makefile.am
> @@ -46,5 +46,20 @@ uninstall-hook:
> -rmdir -p $(DESTDIR)$(bindir)/rhgb-client >& /dev/null
> endif
>
> +if ENABLE_UPSTART
> +plymouth_PROGRAMS += plymouth-upstart-bridge
> +
> +plymouth_upstart_bridge_CFLAGS = $(PLYMOUTH_CFLAGS)
> +plymouth_upstart_bridge_LDADD = \
> + $(PLYMOUTH_LIBS) \
> + $(CURSES_LIBS) \
> + ../libply/libply.la
> +plymouth_upstart_bridge_SOURCES = \
> + $(srcdir)/../ply-boot-protocol.h
> \
> + $(srcdir)/ply-boot-client.h
> \
> + $(srcdir)/ply-boot-client.c
> \
> + $(srcdir)/plymouth-upstart-bridge.c
> +endif
This is independent enough, that I think it should probably be in its own
upstart-monitor or upstart-bridge directory alongside viewer and client, I
think.
> +#include "ply-upstart.h"
i'd call this ply-upstart-monitor.h
> +
> +typedef struct
> +{
> + ply_event_loop_t *loop;
> + ply_boot_client_t *client;
> + ply_upstart_t *upstart;
ply_upstart_monitor_t
> + ply_command_parser_t *command_parser;
> +} state_t;
> +
> +static void
> +dummy_handler (void *user_data, ply_boot_client_t *client)
> +{
> +}
[...]
> +static void
> +update_status (state_t *state,
> + ply_upstart_job_properties_t *job,
> + ply_upstart_instance_properties_t *instance,
> + const char *action, bool ok)
> +{
> + int xenl;
> + const char *hpa;
> +
> + ply_boot_client_update_daemon (state->client, job->name,
> + dummy_handler, NULL, state);
This points out a weakness in the ply_boot_client api. It'd probably just
ditch dummy_handler and change ply_boot_client_update_daemon to allow NULL
there.
[...]
> + xenl = tigetflag ("xenl");
> + hpa = get_string_capability ("hpa");
> +
> + if (xenl > 0 && hpa)
> + {
> + int cols, col;
> + char *hpa_out;
> +
> + cols = tigetnum ("cols");
> + if (cols < 6)
> + cols = 80;
> + col = cols - 7;
> +
> + hpa_out = tiparm (hpa, col);
> + fputs (hpa_out, stdout);
> +
> + if (ok)
> + puts ("[ OK ]");
> + else
> + {
> + const char *setaf, *op;
> + char *setaf_out = NULL;
> +
> + setaf = get_string_capability ("setaf");
> + if (setaf)
> + setaf_out = tiparm (setaf, 1); /* red */
> + op = get_string_capability ("op");
> +
> + printf ("[%sfail%s]\n", setaf_out ? setaf_out : "", op ? op : "");
> + }
> + }
> + else
> + {
> + if (ok)
> + puts (" ...done.");
> + else
> + puts (" ...fail!");
> + }
> +}
I think most of this can be done with ply_terminal methods
(get_number_of_columns, supports_color, set_foreground_color, etc) Again, using
ncurses is fine, I just wanted to let you know about the existing functions in
case you hadn't already evaluated using them.
[...]
> +static void
> +on_disconnect (state_t *state)
> +{
> + ply_error ("error: unexpectedly disconnected from boot status daemon");
> +
> + ply_trace ("disconnect");
> + ply_event_loop_exit (state->loop, 2);
> +}
won't disconnections happen expectedly (when some other client calls plymouth
quit)?
> diff --git a/src/libply/Makefile.am b/src/libply/Makefile.am
> index 50b4d06..3e30845 100644
> --- a/src/libply/Makefile.am
> +++ b/src/libply/Makefile.am
> @@ -50,4 +50,11 @@ libply_la_SOURCES = ply-event-loop.c
> \
> ply-trigger.c \
> ply-utils.c
>
> +if ENABLE_UPSTART
> +libply_la_CFLAGS += $(DBUS_CFLAGS)
> +libply_la_LDFLAGS += $(DBUS_LIBS)
> +libply_HEADERS += ply-upstart.h
> +libply_la_SOURCES += ply-upstart.c
> +endif
> +
Putting this in libply doesn't make a lot of sense. libply is
supposed to be a runtime library ala
glib, nspr, apr, libnih, etc. (actually if hindsight were 20/20, I
probably would have just used glib)
I'd just toss these files into the same directory as the main program
(in the same way ply-boot-client.[ch] is in client/).
> --- a/src/libply/ply-event-loop.c
> +++ b/src/libply/ply-event-loop.c
> @@ -104,6 +104,12 @@ typedef struct
> void *user_data;
> } ply_event_loop_timeout_watch_t;
>
> +typedef struct
> +{
> + ply_event_loop_idle_handler_t handler;
> + void *user_data;
> +} ply_event_loop_idle_closure_t;
> +
[...]
> +void
> +ply_event_loop_watch_for_idle (ply_event_loop_t *loop,
> + ply_event_loop_idle_handler_t idle_handler,
> + void *user_data)
> +{
[...]
> +}
> +
> +void
> +ply_event_loop_stop_watching_for_idle (ply_event_loop_t *loop,
> + ply_event_loop_idle_handler_t
> idle_handler,
> + void
> *user_data)
> +{
[...]
> +}
I'm a little leery about this. From the name, i'm assuming this comprable to
glib idle handlers.
Traditionally glib idle handlers have been used for two purposes:
1) defer work until a "little later" after the main loop has run
2) to hammer the cpu calling the same function over and over again in a loop
The former use case makes sense, but we should give the concept a better name
than idle. (say the api could be
ply_event_loop_watch_for_pending_events_processed, not exactly
sure what the closure object could be called) and make it a one
shot deal.
The latter case rarely makes sense, so I would like to avoid adding api to do
that kind of thing if at all possible.
> index 0000000..bb1bbd8
> --- /dev/null
> +++ b/src/libply/ply-upstart.c
[...]
> +/* Remove an entry from a hashtable, free the key, and return the data.
> + * Memory pools would make this so much easier!
> + */
> +static void *
> +hashtable_remove_and_free_key (ply_hashtable_t *hashtable, const void *key)
> +{
> + void *reply_key, *reply_data;
> +
> + if (!ply_hashtable_lookup_full (hashtable, (void *) key,
> + &reply_key, &reply_data))
> + return NULL;
> + ply_hashtable_remove (hashtable, (void *) key);
> + free (reply_key);
> +
> + return reply_data;
> +}
Can you elaborate on what you'd like to do here instead? If something is
cumbersome to use, we can fix it.
[...]
> +static DBusHandlerResult
> +message_handler (DBusConnection *connection, DBusMessage *message, void
> *data)
> +{
[...]
> +}
This function looks fine from a brief look (as did all the stuff above it), but
I would recommend breaking the guts of each conditional out into separate
subroutines. That function is really long, and most of its guts are
independent chunks of code.
> +static void
> +watch_handler (void *data, int fd)
> +{
> + DBusWatch *watch = data;
> +
> + assert (watch != NULL);
> +
> + /* ply_event_loop doesn't tell us which flags were set; just asserting all
> + * of them should generally work OK, though.
> + */
> + dbus_watch_handle (watch, DBUS_WATCH_READABLE | DBUS_WATCH_WRITABLE);
> +}
> +
Two possible ideas here:
- have two handlers (one for each condition) and call dbus_watch_handle twice
- just fix the event loop code to tack on the status as a third
argument to the handler.
Shouldn't break existing users, I think.
> +static void
> +idle (void *data, ply_event_loop_t *loop)
> +{
> + ply_upstart_t *upstart = data;
> +
> + assert (upstart != NULL);
> +
> + while (dbus_connection_dispatch (upstart->connection) ==
> + DBUS_DISPATCH_DATA_REMAINS)
> + ;
> +}
I think instead of doing this continuously on idle, you should call
dbus_connection_set_dispatch_status_function
and when it has DBUS_DISPATCH_DATA_REMAINS then call dbus_connection_dispatch.
It can't be called from the status function, though, so we will need some sort
of "run it later" function.
That could mean your idle stuff above (reworked to be one shot with a better
name), or you could add an eventfd() or pipe() to the event loop and write to
it from set_dispatch_status_function.
or the even lamer
ply_event_loop_watch_for_timeout (loop, 0.001, ...);
which we shamefully do in other parts of the code...
--Ray
------------------------------
_______________________________________________
plymouth mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/plymouth
End of plymouth Digest, Vol 26, Issue 2
***************************************
