[Xcb] [PATCH proto 1/1] Replace valueparam with switch-bitcase in Xproto
Christian Linhart
chris at DemoRecorder.com
Fri Dec 26 04:27:04 PST 2014
Hi Jaya,
Thank you for this patch.
This will be API and ABI compatible for the same reasons as
your patch for the Render-extension.
I have checked this against the spec and implementation
and have not found any bugs.
Here are some explanations on how to overcome some difficulties
on reading the spec and the implementation.
The protocol specification is in some way misleading
and self-contradicting, but this can be resolved by
looking at the implementation.
Here are some problems with the spec:
The size of the whole value list is correctly described in
http://cgit.freedesktop.org/xorg/proto/xproto/tree/specs/encoding.xml?id=xproto-7.0.26
as
4n LISTofVALUE value-list
i.e. 4 bytes per value, where n is the number of bits set in the mask.
But some of the values themselves are describe incorrectly,
e.g. BITGRAVITY is described as 1 byte value.
This is not correct for two reasons:
* it contradicts LISTofVALUE ( except if implicit padding to 4 byte boundary after each value is assumed )
* even if implicit 4-byte padding is assumed, this contradicts the Xlib implementation in
http://cgit.freedesktop.org/xorg/lib/libX11/tree/src/Window.c?id=libX11-1.6.2#n55
where bit_gravity is assigned to an value of type "unsigned long", which is a 32-bit value
that's then sent verbatim to the server.
Depending on byte-order this is not the same as a 1-byte BOOL and a 3 byte padding.
* The server also treats BITGRAVITY as a 32-bit value.
There, all values from the value list are treated as values of type XID,
which is also 32 bit.
The protocol data is cast to an array of XID.
http://cgit.freedesktop.org/xorg/xserver/tree/dix/dispatch.c?id=xorg-server-1.16.3#n648
Later, a XID* is first dereferenced and then cast to an 8-bit type ( CARD8 ).
http://cgit.freedesktop.org/xorg/xserver/tree/dix/window.c?id=xorg-server-1.16.3#n1195
Please not that it is first dereferenced. Therefore a 32-bit value is retrieved from the given data.
Only after that it is cast to 8-bit, so that the least-significant 8 bits are taken.
That's not the same a 1 byte value plus 3 byte padding depending on byteorder.
Maybe the spec should be changed, so that all values of the valuelist are
described as 4 byte?
***
Anyways, your patch looks good.
Therefore:
Reviewed-by: Christian Linhart <chris at demorecorder.com>
Regards,
Chris
P.S.: I think this should be tested with a testcase
to make sure that no bugs have slipped through.
Especially ABI and API compatibility is important.
Any volunteers to write testcases and do the tests?
Are there existing testcases or testsuites that are suitable to test that?
On 12/22/14 19:35, Jaya Tiwari wrote:
> Changed valueparam to switch bitcase in Xproto for the requests :
>
> CreateWindow
> ChangeWindowAttributes
> ConfigureWindow
> CreateGC
> ChangeGC
> ChangeKeyboardControl
>
> The changes of valueparam to switch has been made as per the specs for
> the extension for the possible values of value-mask and value-list
>
> CreateWindow:
> http://cgit.freedesktop.org/xorg/proto/xproto/tree/specs/encoding.xml#n979
>
> ChangeWindowAttributes:
> http://cgit.freedesktop.org/xorg/proto/xproto/tree/specs/encoding.xml#n1006
>
> ConfigureWindow:
> http://cgit.freedesktop.org/xorg/proto/xproto/tree/specs/encoding.xml#n1105
>
> CreateGC:
> http://cgit.freedesktop.org/xorg/proto/xproto/tree/specs/encoding.xml#n1815
>
> ChangeGC:
> http://cgit.freedesktop.org/xorg/proto/xproto/tree/specs/encoding.xml#n1909
>
> ChangeKeyboardControl:
> http://cgit.freedesktop.org/xorg/proto/xproto/tree/specs/encoding.xml#n2547
>
> Casted KEYCODE32 as a CARD32 and added BOOL32 here instead of glx due
> common to usage in other extensions.
>
> Signed-off-by: Jaya Tiwari <tiwari.jaya18 at gmail.com>
> ---
> src/xproto.xml | 414 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 395 insertions(+), 19 deletions(-)
>
> diff --git a/src/xproto.xml b/src/xproto.xml
> index bfb8a4c..7923b09 100644
> --- a/src/xproto.xml
> +++ b/src/xproto.xml
> @@ -57,6 +57,7 @@ <struct name="CHAR2B">
> <type>GCONTEXT</type>
> </xidunion>
>
> + <typedef oldname="CARD32" newname="BOOL32" />
> <typedef oldname="CARD32" newname="VISUALID" />
>
> <typedef oldname="CARD32" newname="TIMESTAMP" />
> @@ -64,6 +65,8 @@ <struct name="CHAR2B">
> <typedef oldname="CARD32" newname="KEYSYM" />
>
> <typedef oldname="CARD8" newname="KEYCODE" />
> +
> + <typedef oldname="CARD32" newname="KEYCODE32" />
>
> <typedef oldname="CARD8" newname="BUTTON" />
>
> @@ -1293,9 +1296,71 @@ <request name="CreateWindow" opcode="1">
> <field type="CARD16" name="border_width" />
> <field type="CARD16" name="class" enum="WindowClass" />
> <field type="VISUALID" name="visual" />
> - <valueparam value-mask-type="CARD32"
> - value-mask-name="value_mask"
> - value-list-name="value_list" />
> + <field type="CARD32" name="value_mask" mask="CW" />
> + <switch name="value_list">
> + <fieldref>value_mask</fieldref>
> + <bitcase>
> + <enumref ref="CW">BackPixmap</enumref>
> + <field type="PIXMAP" name="background_pixmap" altenum="BackPixmap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BackPixel</enumref>
> + <field type="CARD32" name="background_pixel" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BorderPixmap</enumref>
> + <field type="PIXMAP" name="border_pixmap" altenum="Pixmap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BorderPixel</enumref>
> + <field type="CARD32" name="border_pixel" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BitGravity</enumref>
> + <field type="CARD32" name="bit_gravity" enum="Gravity"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">WinGravity</enumref>
> + <field type="CARD32" name="win_gravity" enum="Gravity"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BackingStore</enumref>
> + <field type="CARD32" name="backing_store" enum="BackingStore"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BackingPlanes</enumref>
> + <field type="CARD32" name="backing_planes" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BackingPixel</enumref>
> + <field type="CARD32" name="backing_pixel" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">OverrideRedirect</enumref>
> + <field type="BOOL32" name="override_redirect" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">SaveUnder</enumref>
> + <field type="BOOL32" name="save_under" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">EventMask</enumref>
> + <field type="CARD32" name="event_mask" mask="EventMask"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">DontPropagate</enumref>
> + <field type="CARD32" name="do_not_propogate_mask" mask="EventMask"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">Colormap</enumref>
> + <field type="COLORMAP" name="colormap" altenum="Colormap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">Cursor</enumref>
> + <field type="CURSOR" name="cursor" altenum="Cursor"/>
> + </bitcase>
> + </switch>
> +
> <doc>
> <brief>Creates a window</brief>
> <description><![CDATA[
> @@ -1374,9 +1439,71 @@ <request name="CreateWindow" opcode="1">
> <request name="ChangeWindowAttributes" opcode="2">
> <pad bytes="1" />
> <field type="WINDOW" name="window" />
> - <valueparam value-mask-type="CARD32"
> - value-mask-name="value_mask"
> - value-list-name="value_list" />
> + <field type="CARD32" name="value_mask" mask="CW" />
> + <switch name="value_list">
> + <fieldref>value_mask</fieldref>
> + <bitcase>
> + <enumref ref="CW">BackPixmap</enumref>
> + <field type="PIXMAP" name="background_pixmap" altenum="BackPixmap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BackPixel</enumref>
> + <field type="CARD32" name="background_pixel" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BorderPixmap</enumref>
> + <field type="PIXMAP" name="border_pixmap" altenum="Pixmap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BorderPixel</enumref>
> + <field type="CARD32" name="border_pixel" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BitGravity</enumref>
> + <field type="CARD32" name="bit_gravity" enum="Gravity"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">WinGravity</enumref>
> + <field type="CARD32" name="win_gravity" enum="Gravity"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BackingStore</enumref>
> + <field type="CARD32" name="backing_store" enum="BackingStore"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BackingPlanes</enumref>
> + <field type="CARD32" name="backing_planes" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">BackingPixel</enumref>
> + <field type="CARD32" name="backing_pixel" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">OverrideRedirect</enumref>
> + <field type="BOOL32" name="override_redirect" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">SaveUnder</enumref>
> + <field type="BOOL32" name="save_under" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">EventMask</enumref>
> + <field type="CARD32" name="event_mask" mask="EventMask"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">DontPropagate</enumref>
> + <field type="CARD32" name="do_not_propogate_mask" mask="EventMask"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">Colormap</enumref>
> + <field type="COLORMAP" name="colormap" altenum="Colormap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="CW">Cursor</enumref>
> + <field type="CURSOR" name="cursor" altenum="Cursor"/>
> + </bitcase>
> + </switch>
> +
> <doc>
> <brief>change window attributes</brief>
> <description><![CDATA[
> @@ -1689,11 +1816,40 @@ <enum name="StackMode">
> <request name="ConfigureWindow" opcode="12">
> <pad bytes="1" />
> <field type="WINDOW" name="window" />
> - <field type="CARD16" name="value_mask" />
> <pad bytes="2" />
> - <valueparam value-mask-type="CARD16"
> - value-mask-name="value_mask"
> - value-list-name="value_list" />
> + <field type="CARD16" name="value_mask" mask="ConfigWindow" />
> + <switch name="value_list">
> + <fieldref>value_mask</fieldref>
> + <bitcase>
> + <enumref ref="ConfigWindow">X</enumref>
> + <field type="INT32" name="x" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="ConfigWindow">Y</enumref>
> + <field type="INT32" name="y" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="ConfigWindow">Width</enumref>
> + <field type="CARD32" name="width" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="ConfigWindow">Height</enumref>
> + <field type="CARD32" name="height" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="ConfigWindow">BorderWidth</enumref>
> + <field type="CARD32" name="border_width" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="ConfigWindow">Sibling</enumref>
> + <field type="WINDOW" name="sibling" altenum="Window"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="ConfigWindow">StackMode</enumref>
> + <field type="CARD32" name="stack_mode" enum="StackMode"/>
> + </bitcase>
> + </switch>
> +
> <doc>
> <brief>Configures window attributes</brief>
> <description><![CDATA[
> @@ -3883,9 +4039,102 @@ <request name="CreateGC" opcode="55">
> <pad bytes="1" />
> <field type="GCONTEXT" name="cid" />
> <field type="DRAWABLE" name="drawable" />
> - <valueparam value-mask-type="CARD32"
> - value-mask-name="value_mask"
> - value-list-name="value_list" />
> + <field type="CARD32" name="value_mask" mask="GC" />
> + <switch name="value_list">
> + <fieldref>value_mask</fieldref>
> + <bitcase>
> + <enumref ref="GC">Function</enumref>
> + <field type="CARD32" name="function" enum="GX"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">PlaneMask</enumref>
> + <field type="CARD32" name="plane_mask" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">Foreground</enumref>
> + <field type="CARD32" name="foreground" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">Background</enumref>
> + <field type="CARD32" name="background" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">LineWidth</enumref>
> + <field type="CARD32" name="line_width" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">LineStyle</enumref>
> + <field type="CARD32" name="line_style" enum="LineStyle"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">CapStyle</enumref>
> + <field type="CARD32" name="cap_style" enum="CapStyle"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">JoinStyle</enumref>
> + <field type="CARD32" name="join_style" enum="JoinStyle"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">FillStyle</enumref>
> + <field type="CARD32" name="fill_style" enum="FillStyle"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">FillRule</enumref>
> + <field type="CARD32" name="fill_rule" enum="FillRule"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">Tile</enumref>
> + <field type="PIXMAP" name="tile" altenum="Pixmap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">Stipple</enumref>
> + <field type="PIXMAP" name="stipple" altenum="Pixmap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">TileStippleOriginX</enumref>
> + <field type="INT32" name="tile_stipple_x_origin" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">TileStippleOriginY</enumref>
> + <field type="INT32" name="tile_stipple_y_origin" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">Font</enumref>
> + <field type="FONT" name="font" altenum="Font"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">SubwindowMode</enumref>
> + <field type="CARD32" name="subwindow_mode" enum="SubwindowMode"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">GraphicsExposures</enumref>
> + <field type="BOOL32" name="graphics_exposures" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">ClipOriginX</enumref>
> + <field type="INT32" name="clip_x_origin" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">ClipOriginY</enumref>
> + <field type="INT32" name="clip_y_origin" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">ClipMask</enumref>
> + <field type="PIXMAP" name="clip_mask" altenum="Pixmap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">DashOffset</enumref>
> + <field type="CARD32" name="dash_offset" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">DashList</enumref>
> + <field type="CARD32" name="dashes" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">ArcMode</enumref>
> + <field type="CARD32" name="arc_mode" enum="ArcMode"/>
> + </bitcase>
> + </switch>
> <doc>
> <brief>Creates a graphics context</brief>
> <description><![CDATA[
> @@ -3924,9 +4173,102 @@ <request name="CreateGC" opcode="55">
> <request name="ChangeGC" opcode="56">
> <pad bytes="1" />
> <field type="GCONTEXT" name="gc" />
> - <valueparam value-mask-type="CARD32"
> - value-mask-name="value_mask"
> - value-list-name="value_list" />
> + <field type="CARD32" name="value_mask" mask="GC" />
> + <switch name="value_list">
> + <fieldref>value_mask</fieldref>
> + <bitcase>
> + <enumref ref="GC">Function</enumref>
> + <field type="CARD32" name="function" enum="GX"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">PlaneMask</enumref>
> + <field type="CARD32" name="plane_mask" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">Foreground</enumref>
> + <field type="CARD32" name="foreground" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">Background</enumref>
> + <field type="CARD32" name="background" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">LineWidth</enumref>
> + <field type="CARD32" name="line_width" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">LineStyle</enumref>
> + <field type="CARD32" name="line_style" enum="LineStyle"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">CapStyle</enumref>
> + <field type="CARD32" name="cap_style" enum="CapStyle"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">JoinStyle</enumref>
> + <field type="CARD32" name="join_style" enum="JoinStyle"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">FillStyle</enumref>
> + <field type="CARD32" name="fill_style" enum="FillStyle"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">FillRule</enumref>
> + <field type="CARD32" name="fill_rule" enum="FillRule"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">Tile</enumref>
> + <field type="PIXMAP" name="tile" altenum="Pixmap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">Stipple</enumref>
> + <field type="PIXMAP" name="stipple" altenum="Pixmap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">TileStippleOriginX</enumref>
> + <field type="INT32" name="tile_stipple_x_origin" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">TileStippleOriginY</enumref>
> + <field type="INT32" name="tile_stipple_y_origin" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">Font</enumref>
> + <field type="FONT" name="font" altenum="Font"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">SubwindowMode</enumref>
> + <field type="CARD32" name="subwindow_mode" enum="SubwindowMode"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">GraphicsExposures</enumref>
> + <field type="BOOL32" name="graphics_exposures" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">ClipOriginX</enumref>
> + <field type="INT32" name="clip_x_origin" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">ClipOriginY</enumref>
> + <field type="INT32" name="clip_y_origin" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">ClipMask</enumref>
> + <field type="PIXMAP" name="clip_mask" altenum="Pixmap"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">DashOffset</enumref>
> + <field type="CARD32" name="dash_offset" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">DashList</enumref>
> + <field type="CARD32" name="dashes" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="GC">ArcMode</enumref>
> + <field type="CARD32" name="arc_mode" enum="ArcMode"/>
> + </bitcase>
> + </switch>
> <doc>
> <brief>change graphics context components</brief>
> <description><![CDATA[
> @@ -4982,9 +5324,43 @@ <enum name="AutoRepeatMode">
>
> <request name="ChangeKeyboardControl" opcode="102">
> <pad bytes="1" />
> - <valueparam value-mask-type="CARD32"
> - value-mask-name="value_mask"
> - value-list-name="value_list" />
> + <field type="CARD32" name="value_mask" mask="KB" />
> + <switch name="value_list">
> + <fieldref>value_mask</fieldref>
> + <bitcase>
> + <enumref ref="KB">KeyClickPercent</enumref>
> + <field type="INT32" name="key_click_percent" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="KB">BellPercent</enumref>
> + <field type="INT32" name="bell_percent" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="KB">BellPitch</enumref>
> + <field type="INT32" name="bell_pitch" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="KB">BellDuration</enumref>
> + <field type="INT32" name="bell_duration" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="KB">Led</enumref>
> + <field type="CARD32" name="led" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="KB">LedMode</enumref>
> + <field type="CARD32" name="led_mode" enum="LedMode"/>
> + </bitcase>
> + <bitcase>
> + <enumref ref="KB">Key</enumref>
> + <field type="KEYCODE32" name="key" />
> + </bitcase>
> + <bitcase>
> + <enumref ref="KB">AutoRepeatMode</enumref>
> + <field type="CARD32" name="auto_repeat_mode" enum="AutoRepeatMode"/>
> + </bitcase>
> + </switch>
> +
> </request>
>
> <request name="GetKeyboardControl" opcode="103">
More information about the Xcb
mailing list