RFC: Addon singleton support

[Rob Taylor]

David Zeuthen wrote:
> Hi,
> 
> Thanks for working on this,
> 
> On Fri, 2007-06-01 at 13:51 +0200, Rob Taylor wrote:
>> Attached is a set of patches that implement singleton addons, roughly as
>> described in doc/TODO. The commit messages should explain everything
>> pretty well..
> 
> I like the simple interface. I tried your patches and it starts up fine;
> however
> 
>  1. when killing the s-addon, the runner dies
> 
>  2. when removing the last device [1] for an s-addon the runner also
>     dies; might be related to item 1.
>

Ah, yes, looks like I failed to update run_exited, I'll fix up the
patch.. (and thanks for the pointer how to test for the last device
removed case..)

>  3. it would be nice if the process line / name for the input s-addon is
>     updated to mention what devices it is listening to

Yeah, I was just being lazy there ;)

>  4. you forgot to attach the patch for the spec :-)

Good point!

>  5. do you think it's necessary to 

to...?

> However it works fine when I add/remove my USB keyboard; the s-addon
> nicely attaches and detaches to handle the device object. So the patch
> more or less works modulo the boundary conditions above.
> 
> Some (brief) comments on the patches itself; I'll promise a more
> thorough review for the next iteration.

Cool, thanks.

>> Introduces a new method StartSingleton on org.freedesktop.HalRunner.
>> This
>> takes one parameter of an array of strings for the environment to
>> launch the
>> singleton. hald-runner keeps a list of running singletons, which are
>> killable
>> only by KillAll.
> 
> Do we really want that? I mean; s-addons are supposed to be used by
> devices not permanently attached; so it may just happen that the last
> device who needs an s-addon is removed; then we should kill the s-addon
> just like we do for normal addon's.

The problem is that its hald-runner's job to kill addons, so to have
hald-runner kill an s-addon would require the addon to communicate
directly with it. IMHO this would just be adding more complexity to
hald-runner than I'd like. The other option is to have hald tell
hald-runner that the singleton is no longer needed with a KillSingleton
method. I didn't go this way for fear of race conditions, but I think
that would be possible, sending KillSingleton after the s-addon has
responded to DeviceRemove.

However, either way it's adding more complexity to hald and hald-runner,
and I don't really see the need as we need a s-addon to be well behaved
anyhow.

> 
>>  GHashTable *udi_hash = NULL;
>> +GList *singletons = NULL;
> 
> Both of these (and all other global variables) should probably be
> declared static. 

aye

>> +void hald_add_device_info (DBusMessageIter * iter, HalDevice *
>> device, char **extra_env);
> 
> We don't seem to define nor use this anywhere.

Strange, must be a bit of left over crud.

> Jumping to singleton_signal_device_changed() in hald/hald_dbus.c :
> 
>> +       //no reply needed, i think
>> +
> 
> Ah, the use of C++ style comments to grab my attention; initially I was
> going to complain about the fact that we should avoid C++ comments; then
> I figured this is actually very important and you grabbed my attention
> that way. Clever indeed!

Heh, purely accidental, I'm afraid ..

> So the way I see it the question is this; should we wait for the
> singleton addon to say "OK, I've poked the device I'm handling, go ahead
> and add it"? Just like we do today for normal addon's; e.g. normal
> addon's today call AddonIsReady() and this allows them to set some
> properties before the device object in question is announced to the
> world. 
> 
> I think we want that for singleton addon's too, e.g. your add_device()
> function in hald/linux/addons/addon-input.c needs to poke the hal daemon
> back. That way an s-addon can set properties on a device object. 
> 
> Thoughts?

Hmm, well it does poke the daemon back, its just an empty reply message,
so setting device properties in the singleton_device_added callback
would  have the desired effect, except for the fact the addon's been
called with a blocking call. This raises a big problem I think we have
in our dbus code - everything is done blocking, we don't use the
dbus-glib mainloop integration to allow async callbacks.

Should I update the patch to at least do this for this particular call?

>> <libhal property set API additions>
> 
> Are you sure we want to add all that API? I agree it's kind of.. well..
> less than useful the way the API is right now with iterators and what
> not. But it forces people to write code that only iterates over the
> properties in a single sweep; picking up what they need. Any chance you
> can change your code to do that? It seems inefficient to do an O(n)
> lookup for the M properties you want.

My thought was actually just to change the implementation to use a
hash-table rather than a list...

> Anyway, hope that's enough comments to get you to the next iteration.
> Most of the things I didn't comment on looked pretty reasonable.
> 
>      David


Thanks,

Rob