[Xcb] Next Release?
Ran Benita
ran234 at gmail.com
Sat Oct 12 15:53:19 PDT 2013
On Sat, Oct 12, 2013 at 04:05:26PM +0200, Daniel Martin wrote:
> On Fri, Oct 11, 2013 at 06:39:27PM +0900, Arnaud Fontaine wrote:
> > Peter Harris <pharris at opentext.com> writes:
> > > On 2013-10-04 06:47, Arnaud Fontaine wrote:
> > >> Peter Harris <pharris at opentext.com> writes:
> > >>
> > >>> My short-term plan, if I ever get a free minute, is to convert the
> > >>> events into structs (deleting the type field) and add one event that
> > >>> contains the type and a union with all the structs.
> > >>
> > >> Done by Daniel Martin but not merged yet:
> > >> http://lists.freedesktop.org/archives/xcb/2013-August/008541.html
> > >>
> > >> Should it be merged?
> > >
> > > Yes, plesae.
> > >
> > > It appears I forgot to review Daniel's patch at the time. I'll do so now:
> > >
> > > Reviewed-by: Peter Harris <pharris at opentext.com>
> > >
> > >> * [Bug 68387] New: xinput2 XIQueryDevice reply not read correctly
> > >> https://bugs.freedesktop.org/show_bug.cgi?id=68387
> > >
> > > Yes, please. Looks like Ran Benita both reviewed and tested this patch.
> >
> > Concerning this patch and the ones above, Daniel Martin asked me to hold
> > off a week to see if can finish his parser rewrite to handle
> > <switch>/<case> in XML description. As this rewrite would break the API,
> > I think it's better to wait a week and see how it goes...
>
> Due to (personal) distractions I'm unable to make it usable anytime
> soon. Sorry for that.
>
> But, I sat down and had a look at that patches and had a look if we
> can get rid of "xkb: Comment out Doodads" with the patch attached to bug
> #68387 "xinput2 XIQueryDevice reply not read correctly".
Those Doodad things really stretch the xcb model. I feel confident in
saying that no one will ever use xcb to handle XKB geometry. If they
cause specific problems we should just ignore them.
> While doing so I've found more issues in the generated xkb code. Those
> issues seem to be a real show stoper, which we can't comment out that
> easily. More on this at the bottom.
>
> > Therefore I have not merged the following patches:
> >
> > * [Bug 68387] New: xinput2 XIQueryDevice reply not read correctly
> > https://bugs.freedesktop.org/show_bug.cgi?id=68387
>
> This seem to be the right thing. As Ran mentioned in the bug the size of
> lists is correct afterwards. Unit tests for other lists might be nice.
> Anyone knows of simple to run applications using xcb and heavily using
> lists out of the head?
Only one I know is cairo-xcb. Not sure about simple to run though, and
not very heavy on the lists neither.
I think the patch should go in, without it many _sizeof() functions are
pretty much broken. Perhaps that's an opportunity to *remove* the
sizeof() functions from the public API. At least that's how they do it
in X, if it's broken so long and nobody noticed... I can look and see
if that's even feasible internally, unless removing these functions is
completely unacceptable.
>
> > * [PATCH proto 1/3] xkb: Comment out Doodads
> > http://lists.freedesktop.org/archives/xcb/2013-August/008539.html
> > => Reviewed-By: Ran Benita <ran234 at gmail.com>
> >
> > * [PATCH proto 3/3] xkb: Unify events into single event
> > http://lists.freedesktop.org/archives/xcb/2013-August/008541.html
> > => Tested-By: Ran Benita <ran234 at gmail.com>
>
> I don't like the patch. If we add it know we'll have an API break when
> replacing the union with a switch/case. Which we can't do atm. due to my
> fault.
>
> Would it be an option to have a planned "break a lot of things - 2.0"
> release? There're other things that could be changed even atm., i.e. the
> valueparams in xproto usually could've been switch/bitcases.
>
> > * [PATCH 1/1] XKB: Fix values of AXFBOpt enum
> > http://lists.freedesktop.org/archives/xcb/2013-September/008668.html
> > => Reviewed-By: Ran Benita <ran234 at gmail.com>
> >
> > * [Xcb] [PATCH 2/1] XKB: Rewrite AXOption
> > http://lists.freedesktop.org/archives/xcb/2013-September/008669.html
> > => Reviewed-By: Ran Benita <ran234 at gmail.com>
>
> Those 2 patches can be merged too.
>
> > Please correct me if I have misunderstood something.
> >
> > Moreover, As per Daniel's request, I have also merged these patches:
> > https://github.com/bartsch/xcb-proto/tree/next-for-master
> >
> > >> * [Bug 69118] New: Unix spool is located at /var/spool/sockets/X11/... or /usr/spool/sockets/X11/... under HPUX
> > >> https://bugs.freedesktop.org/show_bug.cgi?id=69118
> > >
> > > Yes, please. The "Simpler patch." version clearly can't hurt anybody not
> > > using HPUX. And anybody still using HPUX is used to hurt by now.
> >
> > Merged, thanks.
>
> Thanks for merging.
>
> Some more problems with xkb:
>
> First, the summary if you don't want to read the whole story: It looks
> like _every_ structure having a fixed size field after a variadic
> field/list is broken! Most other extensions are not affected as they've
> the variadic stuff at the end of a structure - not intermixed with fixed
> size fields.
>
> Most and easy example - xkb/struct[name=Property]:
> http://cgit.freedesktop.org/xcb/proto/tree/src/xkb.xml#n558
>
> This struct has a
> - fixed size field (nameLength)
> - variadic list (name)
> - fixed size field (valueLength)
> - variadic list (value).
>
> Now, look at the generated structure:
> typedef struct xcb_xkb_property_t {
> uint16_t nameLength;
> uint16_t valueLength; <-- doesn't belong here
> } xcb_xkb_property_t;
(The definition of this one is actually wrong, there's a server/spec
mismatch here if you remember:
http://lists.freedesktop.org/archives/xcb/2013-August/008448.html).
I'm not sure what *should* happen in this case, either way it comes out
awkward, asymmetric, or gives an empty struct. Note again that this is
part of XKB geometry (which we dropped support for in xkbcommon because
it is so baroque and unmaintained and little used).
>
> valueLength is not supposed to show up in the generated structure!
> The generator should've created an accessor for this field (taking
> the prior 'name' list into account. Event worse in this case, the
> default naming scheme within the code generator would choose a name
> for the accessor which conflicts with length function for the
> 'value' list. Both would be called xcb_xkb_property_value_length.
> (Yes, valueLength and nameLength need to be renamed, but imho the
> generator doesn't detect such conflicts.)
>
> Another example I just briefly point at - xkb/request[name=GetKbdByName]:
> http://cgit.freedesktop.org/xcb/proto/tree/src/xkb.xml#n2032
>
> typedef struct xcb_xkb_get_kbd_by_name_request_t {
> uint8_t major_opcode;
> uint8_t minor_opcode;
> uint16_t length;
> xcb_xkb_device_spec_t deviceSpec;
> uint16_t need;
> uint16_t want;
> uint8_t load;
> uint8_t pad0;
> uint8_t keymapsSpecLen;
>
> keymapsSpecLen is the last fixed size field and the last field
> having a fixed position in the request. Everything below is
> wrong.
>
> uint8_t keycodesSpecLen;
> uint8_t typesSpecLen;
> uint8_t compatMapSpecLen;
> uint8_t symbolsSpecLen;
> uint8_t geometrySpecLen;
> } xcb_xkb_get_kbd_by_name_request_t;
>
> I haven't tried to send the request. It could work until you specify
> some spec field/name. But, the problem remains.
For the request it might do the right thing, I don't think it sends the
struct directly but pushes it through a serialize function. But I won't
be trying this one any time soon, takes too much time..
>
>
> Atm. I'm looking at c_client.py how to fix that. (And to compensate my
> lack of a rewrite so far.)
Thanks for all your work.
Ran
More information about the Xcb
mailing list