Plymouth/Upstart integration
Colin Watson
cjwatson at ubuntu.com
Wed Jan 12 08:17:06 PST 2011
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: <http://lists.freedesktop.org/archives/plymouth/attachments/20110112/d380e79a/attachment-0001.patch>
More information about the plymouth
mailing list