RFC: Addon singleton support

[David Zeuthen]

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.

 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

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

 5. do you think it's necessary 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.

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


>  GHashTable *udi_hash = NULL;
> +GList *singletons = NULL;

Both of these (and all other global variables) should probably be
declared static. 

> +void hald_add_device_info (DBusMessageIter * iter, HalDevice *
> device, char **extra_env);

We don't seem to define nor use this anywhere.

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!

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?

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

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

[1] : I used a patch like this to restrict this to only match on my
specific Logitech USB keyboard

--- a/fdi/policy/10osvendor/10-input-policy.fdi
+++ b/fdi/policy/10osvendor/10-input-policy.fdi
@@ -4,17 +4,21 @@
 
   <device>
     <match key="info.capabilities" contains="input">
-      <match key="info.capabilities" contains="button">
-       <match key="info.addons.singleton" contains_not="hald-addon-input">
-         <append key="info.addons.singleton" type="strlist">hald-addon-input</append>
-        </match>
-      </match>
-      <match key="info.capabilities" contains="input.keys">
-       <match key="info.addons.singleton" contains_not="hald-addon-input">
-         <append key="info.addons.singleton" type="strlist">hald-addon-input</append>
-        </match>
-        <match key="info.capabilities" contains_not="button">
-         <append key="info.capabilities" type="strlist">button</append>
+      <match key="@input.originating_device:usb.vendor_id" int="0x046d">
+        <match key="@input.originating_device:usb.product_id" int="0xc512">
+          <match key="info.capabilities" contains="button">
+           <match key="info.addons.singleton" contains_not="hald-addon-input">
+             <append key="info.addons.singleton" type="strlist">hald-addon-input</append>
+            </match>
+          </match>
+          <match key="info.capabilities" contains="input.keys">
+           <match key="info.addons.singleton" contains_not="hald-addon-input">
+             <append key="info.addons.singleton" type="strlist">hald-addon-input</append>
+            </match>
+            <match key="info.capabilities" contains_not="button">
+             <append key="info.capabilities" type="strlist">button</append>
+            </match>
+          </match>
         </match>
       </match>
     </match>