[Xcb] [PATCH proto xkb] Fixes for xkb.xml
Christian Linhart
chris at DemoRecorder.com
Wed Feb 12 15:26:57 CET 2014
Hi Ran,
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.
Do you have good testcases for xcb-xkb?
If yes, please send them to me and I'll use them in my tests.
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.
- "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.
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? )
>> <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.
>
>> <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.
>
>> <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.
***
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.
Chris
>
More information about the Xcb
mailing list