RFC: Addon singleton support

[Rob Taylor]

Set of updated patches attached. I haven't got a USB keyboard to hand to
 test last-item removal, but it should be fixed now.

Also attached is a patch to make LibHalPropertySet a hashtable. I used
uthash.h [1], it's not exactly the tidiest hashtable implementation, but
it was the most capable well-licensed implementation I could find.

Thanks,
Rob

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.
> 
>  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>
> 
> 
> _______________________________________________
> hal mailing list
> hal at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/hal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-add-hald-runner-support-for-singleton-addons.patch
Type: text/x-patch
Size: 0 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/hal/attachments/20070607/375abdc1/attachment-0005.bin