Plymouth/Upstart integration

[Colin Watson]

On Tue, Dec 14, 2010 at 05:56:05PM -0500, Ray Strode wrote:
> (one the one month anniversary of your message...wheee! sorry about that)

Not that I responded to it very much quicker ...

> On Sun, Nov 14, 2010 at 10:23 AM, Colin Watson <cjwatson at ubuntu.com> wrote:
> > +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

Done (--enable-upstart-monitoring).

> 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/.

See below.

> Why aren't you using pkgconfig for ncurses though?

The Ubuntu package doesn't have a pkg-config file - that seems to be a
configure option to ncurses.  I've filed a Debian bug about including
that, but in any case the fact that the presence of the pkg-config file
depends on a configure option probably means that it isn't terribly
portable.

> 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.

Done (upstart-bridge).

> > +#include "ply-upstart.h"
> i'd call this ply-upstart-monitor.h

Done.

> > +  ply_upstart_t        *upstart;
> ply_upstart_monitor_t

Done.

> > +  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.

Done.

> 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.

I hadn't noticed ply_terminal, so thanks.  However, it doesn't seem very
good at doing things like moving the cursor position, and I'm concerned
about eventual support for other console types (in particular serial
consoles).  I realise there's probably some way to go before that works
well, but I'd like to avoid putting more obstacles in the way.
Importing terminfo support seems like not a whole lot of fun - ncurses
should be better at that.

> [...]
> > +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)?

Good point.  Fixed (removed ply_error, expanded ply_trace, set exit code
to 0).

> > 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'm sort of glad you didn't.  Lobbing an extra 800K+ of library code
into our initramfs would slow things down and make me sad, and applying
library reduction to glib sounds like an exercise in pain. :-)

> 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/).

Done.

> 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.

Based on your feedback below about dispatch status functions, I've
removed this API.

> > +/* Remove an entry from a hashtable, free the key, and return the data.
> > + * Memory pools would make this so much easier!
> > + */
> > +static void *
> Can you elaborate on what you'd like to do here instead?  If something is
> cumbersome to use, we can fix it.

It was really just a whine to the effect that a memory pool approach
means that freeing the top-level handle for the hashtable could
automatically free everything attached to it, rather than having to do
it manually.  That's quite a bit of infrastructure, though - the
hashtable implementations in my own projects don't bother - so I didn't
really expect this to be worth fixing!

I've just removed the whine from the comment.

> > +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.

Done.

> > +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.

I looked at both, thanks - the former feels clearer to me anyway, so
have done that.

> > +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.

Mm, I'm not sure why I didn't notice
dbus_connection_set_dispatch_status_function.

An eventfd sounds like a good plan; I would rather not wake up the event
loop unnecessarily.  Done.  This also exposed a few ordering problems
elsewhere in my event handling, which I've fixed.

How's this?

Thanks,

-- 
Colin Watson                                       [cjwatson at ubuntu.com]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: plymouth-upstart.patch
Type: text/x-diff
Size: 59048 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/upstart-devel/attachments/20110112/d380e79a/attachment-0001.patch>