[patch] Re: HAL and hotplugging

[Daniel Stone]

On Sat, Mar 15, 2008 at 05:36:47PM -0400, Dustin Spicuzza wrote:
> Ok, this patch allows the HAL layer to send arbitrary options to the  
> driver. I tested it using the evtouch driver that my touchscreen uses...  
> but it'll most likely work with evdev or any other driver too. Test it  
> and let me know.
>
> Also, I changed some of the output statements to ErrorF instead of  
> DebugF, since I couldn't figure out how to display the DebugF  
> statements, and it was annoying to not be able to see them (while right  
> now my changes made it more verbose, at least it gives you the ability  
> to figure out what isn't working, and that HAL is actually *trying* to  
> do something, as opposed to how it was before. Theres probably a better  
> solution...
>
> Also, I know strncasecmp isn't standard.. and I used it. Is that an issue?

When we hit a platform that doesn't have it, we can steal the BSD libc's
implementation.  Until then, I wouldn't really worry -- I guess, just
add a configure check that looks for it and bombs out if not.

> -    DebugF("[config/hal] removing device %s\n", dev->name);
> +    /* this isn't an error, but the user should see this */
> +    ErrorF("[config/hal] removing device %s\n", dev->name);

Hmm, do they really need the log filled with messages every time they
plug or unplug a device?

> +	LibHalPropertySet *set = NULL;
> +    char *psi_key = NULL, *tmp_val;

Please, no tabs. :)

> @@ -192,7 +200,7 @@ device_added(LibHalContext *hal_ctx, const char *udi)
>      driver = get_prop_string(hal_ctx, udi, "input.x11_driver");
>      path = get_prop_string(hal_ctx, udi, "input.device");
>      if (!driver || !path) {
> -        DebugF("[config/hal] no driver or path specified for %s\n", udi);
> +        ErrorF("[config/hal] no driver or path specified for %s\n", udi);

Err, assuming you remove the capabilities check, this will trigger every
single time you hotplug any device that isn't an input device, e.g. a
thumbdrive, camera, external media, etc, etc.

> @@ -205,17 +213,22 @@ device_added(LibHalContext *hal_ctx, const char *udi)
>          xkb_layout = get_prop_string(hal_ctx, udi, "input.xkb.layout");
>          xkb_variant = get_prop_string(hal_ctx, udi, "input.xkb.variant");
>          xkb_options = get_prop_string_array(hal_ctx, udi, "input.xkb.options");
> -    }
> +    } 

Please don't add trailing whitespace.

> +	/* most drivers use device.. not path. evdev uses both however, but the 
> +     * path version isn't documented apparently. support both for now. */
>      add_option(&options, "path", path);
> +    add_option(&options, "device", path);

Yeah.  Given that there are so few hotplug-capable drivers, though
(evdev, evtouch, wacom, ... ?), I was hoping that we'd just be able to
change the drivers to take the slightly more explicit 'path'.

> +    /* ok, grab any other options from hal.. iterate through all properties
> +    * and lets see if any of them are options that we can add */
> +    set = libhal_device_get_all_properties(hal_ctx, udi, &error);

Why not just fold all the above code into the iterator here, so we cut
down on round trips?

> +    /* this isn't an error, but how else do you output something that the user can see? */
> +    ErrorF("[config/hal]: Adding input device %s\n", name);

Yeah, ErrorF is a pretty spectacular misnomer.

> +    <!-- The way this works: 

Thanks for the commenting! :)

Looks pretty much good other than the above comments.

Cheers,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg/attachments/20080316/aa6acf23/attachment.pgp>