[Xcb] [PATCH proto xkb] Fixes for xkb.xml
Ran Benita
ran234 at gmail.com
Thu Feb 13 12:06:51 CET 2014
On Wed, Feb 12, 2014 at 03:26:57PM +0100, Christian Linhart wrote:
> Hi Ran,
Hi
> Thank you very much for your comments and questions.
>
> I have inserted my answers below.
>
> On 02/11/14 21:02, Ran Benita wrote:
> >On Tue, Feb 11, 2014 at 05:50:25PM +0100, Christian Linhart wrote:
> >> From 29a885d4310ab1e8626abbca14dc60af79365371 Mon Sep 17 00:00:00 2001
> >>From: Christian Linhart<chris at demorecorder.com>
> >>Date: Tue, 11 Feb 2014 15:39:51 +0100
> >>Subject: [PATCH] xkb protocol fixes and reenable features
> >>
> >>* remove keymapsSpec from the requests ListComponents and GetKbdByName
> >>* reenable previously commented-out features such as Doodads
> >>---
> >> src/xkb.xml | 41 +++--------------------------------------
> >> 1 file changed, 3 insertions(+), 38 deletions(-)
> >>
> >>diff --git a/src/xkb.xml b/src/xkb.xml
> >>index 9ef4402..6e37c44 100644
> >>--- a/src/xkb.xml
> >>+++ b/src/xkb.xml
> >>@@ -547,26 +547,10 @@ authorization from the authors.
> >>
> >> <typedef oldname="char" newname="STRING8" />
> >>
> >>- <!-- XXX: Property is broken
> >>- <struct name="Property">
> >>- <field name="nameLength" type="CARD16" />
> >>- <list name="name" type="STRING8">
> >>- <fieldref>nameLength</fieldref>
> >>- </list>
> >>- <field name="valueLength" type="CARD16" />
> >>- <list name="value" type="STRING8">
> >>- <fieldref>valueLength</fieldref>
> >>- </list>
> >>- </struct>
> >>- -->
> >>- <!-- XXX: This would be the correct Property structure as per spec.,
> >>- but it's broken atm. too. Add it anyway here, so we don't
> >>- loose that information.
> >These parts were commented out only recently, please see the git history
> >and the mailing list discussions accompanying these patches (you can use
> >'git blame src/xkb.xml' to see the commits which did the commenting-out).
> >Why do you believe they work now?
>
> Reason 1. I have tested this with my own code-generator which translates IDs in an X11-proxy.
> It works with xkb.xml with my patches, but it does not work with version 1.9.1, i.e., before these things were commented out.
> I have tested this e.g. with xkbprint which calls requests which have Doodads in their replys. ( GetXkbByName as far as I remember )
>
> Maybe my code-generator does something different from the xcb-code-generator?
> I'll check that and will post the results of my tests here.
OK, I'll wait until your next patch.
> Do you have good testcases for xcb-xkb?
> If yes, please send them to me and I'll use them in my tests.
No, there isn't a test suite or something. I did write a test program
for GetGeometry once, which compared the results with Xlib. I've
attached it now (geomtest.c) though note I haven't compiled or run it
since (because the geometry stuff was commented out). Maybe you can use
it and make it more extensive.
Then there is also code which uses xcb-xkb, which you can use to make
sure there are no regressions. For example, qt5 XCB backend, and
xkbcommon-x11 library:
https://raw.github.com/xkbcommon/libxkbcommon/master/src/x11/keymap.c
But these don't use XkbGetGeometry or XkbGetKbdByName.
> Reason 2. There were several fixes after Doodads etc were commented out, so the could have fixed the original problems:
> Especially the following ones:
>
> - "xkb: Use <pad align="4" /> in GetMap"
> This adds proper alignment in several places as far as I can tell.
I would have to look at the xml and see if it's OK.
The libxcb code generator can't handle Doodads currently, that's why we
commented it out. But if the xml is OK, and there are other code
generators which *can* handle it, then I guess we should indeed
uncomment them. It didn't really occur to me TBH..
> - "xkb: Add struct Property as per spec (commented out)"
> I have taken the definition of "struct Property" from this commit, so this is a change to the prior definition.
Same here. For libxcb, see:
https://bugs.freedesktop.org/show_bug.cgi?id=71758
>
> Reason 3. I have compared this with X-Server source, and the result is: the spec of Doodads and Property in the XML matches what the X-Server does.
> ( Did I overlook something? )
I would have to take some time to look at it. It would be very helpful
if you can send each logical change as a separate patch with it's own
rationale, then it is easier to review piece by piece, and also easier
to bisect in case something breaks. Don't worry if it comes out as 10
patches..
> >> <struct name="Property">
> >> <field name="name" type="CountedString16" />
> >> <field name="value" type="CountedString16" />
> >> </struct>
> >>- -->
> >>
> >> <struct name="Outline">
> >> <field name="nPoints" type="CARD8" />
> >>@@ -643,7 +627,6 @@ authorization from the authors.
> >> <item name="Logo"> <value>5</value> </item>
> >> </enum>
> >>
> >>- <!-- XXX: doodads are broken
> >Doodads are particularly troublesome as far as XCB and the code
> >generator go. They do not work properly, please see this thread:
> >http://lists.freedesktop.org/archives/xcb/2013-August/008448.html
> I am aware of discussions about xkb-problems but didn't look as far back as August.
> It seems to me that there may be some work to do in the xcb code generator to get this working.
>
> So, my patch should not be applied until the code-generator issue is analyzed.
> (any my patch needs some fixes anyways, see further below)
>
> Maybe my tests will reveal something which can be improved in the xcb code generator.
>
> I would be good if we get xkb working, and I am willing to help.
As I mentioned, as I see it the XMLs are not meant to serve libxcb
alone, so if it's just libxcb that's broken and not the XMLs, we should
reconsider whether to comment out these pieces. That said, if you can
help in any way to make the libxcb's code generator handle XKB better,
that would be great.
> >
> >> <struct name="CommonDoodad">
> >> <field name="name" type="ATOM" />
> >> <field name="type" type="CARD8" enum="DoodadType" />
> >>@@ -737,7 +720,6 @@ authorization from the authors.
> >> <fieldref>nOverlays</fieldref>
> >> </list>
> >> </struct>
> >>- -->
> >>
> >> <struct name="Listing">
> >> <field name="flags" type="CARD16" />
> >>@@ -1829,7 +1811,6 @@ authorization from the authors.
> >> </switch>
> >> </request>
> >>
> >>- <!-- XXX: Property and doodads are broken, which renders GetGeometry useless
> >> <request name="GetGeometry" opcode="19">
> >> <field name="deviceSpec" type="DeviceSpec" />
> >> <pad bytes="2" />
> >>@@ -1870,9 +1851,7 @@ authorization from the authors.
> >> </list>
> >> </reply>
> >> </request>
> >>- -->
> >>
> >>- <!-- XXX: Property and doodads are broken, which renders SetGeometry useless
> >> <request name="SetGeometry" opcode="20">
> >> <field name="deviceSpec" type="DeviceSpec" />
> >> <field name="nShapes" type="CARD8" />
> >>@@ -1907,7 +1886,6 @@ authorization from the authors.
> >> <fieldref>nKeyAliases</fieldref>
> >> </list>
> >> </request>
> >>- -->
> >>
> >> <request name="PerClientFlags" opcode="21">
> >> <field name="deviceSpec" type="DeviceSpec" />
> >>@@ -1930,11 +1908,8 @@ authorization from the authors.
> >> <request name="ListComponents" opcode="22">
> >> <field name="deviceSpec" type="DeviceSpec" />
> >> <field name="maxNames" type="CARD16" />
> >>- <!-- XXX: Intermixed fixed size fields and lists are broken
> >>- <field name="keymapsSpecLen" type="CARD8" />
> >>- <list name="keymapsSpec" type="STRING8">
> >>- <fieldref>keymapsSpecLen</fieldref>
> >>- </list>
> >>+ <!-- CL: there is no list named keymapsSpec in the request
> >>+ ( only the reply contains a list of keymaps) -->
> >Why do you say that?
> >The xkbproto spec says:
> >
> >1 CARD8 opcode
> >1 22 xkb-opcode
> >2 2+(6+m+k+t+c+s+g+p)/4 request-length
> >2 KB_DEVICESPEC deviceSpec
> >2 CARD16 maxNames
> >1 m keymapsSpecLen
> >m STRING keymapsSpec
> >1 k keycodesSpecLen
> >k STRING keycodesSpec
> >1 t typesSpecLen
> >t STRING typesSpec
> >1 c compatMapSpecLen
> >c STRING compatMapSpec
> >1 s symbolsSpecLen
> >s STRING symbolsSpec
> >1 g geometrySpecLen
> >g STRING geometrySpec
> >p unused,p=pad(6+m+k+t+c+s+g)
> >
> >And this is what libX11 sends as well. Recently support for that request
> >was removed from the server entirely, i.e. it just gives 0's for all the
> >items back (the request was apparently broken for a long time and nobody
> >noticed).
>
> Thank you for these infos. I have checked Xlib, too, now.
> The XServer does not parse this list, but Xlib sends it, so the X-Server gets out of sync when interpreting this request.
> So this could not have been worked.
> Therefore it was probably not used by anybody.
> So, removing the request from the server is OK.
> If somebody needs the request, it can be readded to the server with a fixed protocol.
>
> In xkb.xml, this request should be commented out with a comment explaining the reason.
> The list should be readded to it.
> I'll make a new version of this patch where this is the case.
I dont' see any reason to comment it, if it's unsupported by the
*server*. Does it cause any problems?
> >
> >> <field name="keycodesSpecLen" type="CARD8" />
> >> <list name="keycodesSpec" type="STRING8">
> >> <fieldref>keycodesSpecLen</fieldref>
> >>@@ -1955,7 +1930,6 @@ authorization from the authors.
> >> <list name="geometrySpec" type="STRING8">
> >> <fieldref>geometrySpecLen</fieldref>
> >> </list>
> >>- -->
> >> <reply>
> >> <field name="deviceID" type="CARD8" />
> >> <field name="nKeymaps" type="CARD16" />
> >>@@ -1993,11 +1967,7 @@ authorization from the authors.
> >> <field name="want" type="CARD16" mask="GBNDetail" />
> >> <field name="load" type="BOOL" />
> >> <pad bytes="1" />
> >>- <!-- XXX: Intermixed fixed size fields and lists are broken
> >>- <field name="keymapsSpecLen" type="CARD8" />
> >>- <list name="keymapsSpec" type="STRING8">
> >>- <fieldref>keymapsSpecLen</fieldref>
> >>- </list>
> >>+ <!-- CL: there is no list named keymapsSpec in the request ( only the reply contains a list of keymaps) -->
> >The spec is similar here, that field is a part of the request, it is
> >just not supported by the server.
>
> This is the request GetKbdByName.
> Here, the situation is different from above.
> You are right. The list is parsed by the server but unsupported. i.e. if it has non-zero length, the server returns BadMatch.
>
> But the <pad bytes="1"> needs to be removed.
> This pad is not in the spec and it is not necessary because keymapsSpec consists of 1 byte values and therefore needs no alignment anyways.
>
> This explains why removing the list has fixed the problem in my tests:
> The pad and the empty list took the same space, so the rest becomes synchronized when either of the two is removed.
>
> I'll fix that and will send another version of that patch after I have done the tests with xcb-codegenerator, too.
> This may take a few days.
Please send this as a separate patch with the above explanation.
> ***
>
> So we are getting closer to a solution but not as fast as I originally thought.
> Thank you for reviewing my patch. This was really helpful towards a solution.
Thanks for looking at this.
Ran
> Chris
More information about the Xcb
mailing list