I've been looking through your patch and your notes. I've got some comments below. If others have comments, please send them too.
On Mon, Jan 17, 2005 at 09:15:45PM +0100, Michal Maru?ka wrote: >Hello. > >i've seen the announcement of the deadline for new code for 4.5, and decided >to present my work. There are some rough edges, but, let me try: > >First of all, the patch is here >http://bugs.xfree86.org/show_bug.cgi?id=1529 >or alternatively http://maruska.dyndns.org/comp/packages/xkb-plugin.patch > > >* MOTIVATION: >The motivation for these patches is to make possible to load a plugin which >rewrites the stream of pressed and released keycodes with possibly some delay, >into other keycodes, in function of the duration and parallelism of the >strokes. The motivation sound fine, and I like the idea. In general it could be useful to allow for manipulation of input streams by allowing modules/plugins to be inserted at different points in the input stream. Digging into a specific case like this is a great way to start. >I have made the patches in a way, so that with no plugin loaded all works as >normal, in fact better due to patches to auto-repeat code, and some XKB >functions (there were bugs in configuration requests server-side, and in >reconfiguration-notification events processing client-side). It would be good to have the bug-fixes separated out. >Limitations/drawbacks: >- one plugin only (per device, but tested only on "one > keyboard"). >- Anyone can request a plugin, just by filename. You might consider using the XFree86 loader mechanisms for loading the plugin, even if you keep it in dlopen() form. It may make it easier to control which modules are permitted to be loaded, which is critically important for a server that runs as root. An alternative would be to strip off all pathname components from 'filename' and prepend the XFree86 server's module path. The XFree86 loader provides a more flexible mechanism for safely locating modules though. >programs/Xserver/hw/xfree86/input/mouse/mouse.c I want the keyboard plugin >to know > about mouse events too. This patch is quite ugly, since it searches for > the keyboard in global variables. Core pointer events should be picked up at a higher level, not here. The mouse driver isn't the only one that can be the core pointer. >I hacked some code to read (on linux) from /dev/input/event* files/devices >(provided >by evdev linux kernel module; for usb keyboards in 2.4, and for all in 2.6 >series). >These (afaik) provide the precise time of event. > >In making this piece of code, my priority was simplicity and security to never >end >up without keyboard. > >files: >programs/Xserver/hw/xfree86/os-support/linux/lnx_io.c ... discovering evdev >devices. > >programs/Xserver/hw/xfree86/os-support/shared/std_kbdEv.c reading from evdev >(with time) > >programs/Xserver/hw/xfree86/common/xf86Priv.h the new function & evdev >structures It would be better to do this, at least in the first instance, via the modular "kbd" driver, and the os-support mechanisms that it uses. In particular, Linux-specific processing should not be in shared/std_kbdEv.c. There are one or two event-based keyboard handlers for other OSs: for example, see WSCONS_SUPPORT in os-support/bsd/. >* time: >The time provided in the evdev events, is the same as returned by >gettimeofday(). I >had to change GetTimeInMillis, unfortunately, reverting a previous patch >to make time monotone. Maybe, whey I drop Timers in favour of a patch to >xf86eqProcessInputEvents there won't be a danger for non-monotoe time (of >events). Maybe a possibly non-monotonic version is needed in addition to the existing one? >* Known issues: > >** hardware (non XKB-generated) auto-repeat. appears to work ok, but i'm not >sure about the > implications of the patch to programs/Xserver/dix/events.c > >** Some parts of the patch should be #ifdef XKB: the auto-repeat code in >programs/Xserver/dix/events.c > >** evdev: I don't know how the select()-related code works. So, I do rely on >xf86Info.consoleFd. > >** security: the plugin should be loadable from restricted directory. > >** installation > to build the plugin, I need some server headers. > for now i do: cp -a programs/Xserver/include/ > ${projectroot}/include/X11/server/ > >** inconsistent code indentation and symbol names. After a read through the patch, I'd say that a significant issue that needs to be addressed is the layering. XFree86 is largely broken into different layers, which give it a good degree of modularity. There are areas in your patch that mix things between some of the layers. I think the first thing to do is to identify the relevant layers and rework the areas that cross them. Thanks for submitting your work! David _______________________________________________ Devel mailing list [email protected] http://XFree86.Org/mailman/listinfo/devel
