Plymouth/Upstart integration
Ray Strode
halfline at gmail.com
Tue Dec 14 14:56:05 PST 2010
Hi,
(one the one month anniversary of your message...wheee! sorry about that)
On Sun, Nov 14, 2010 at 10:23 AM, Colin Watson <cjwatson at ubuntu.com> 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
More information about the plymouth
mailing list