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 -- upstart-devel mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/upstart-devel
