[Xcb] EWMH API for XCB

[Jamey Sharp]

On Tue, Dec 15, 2009 at 01:13:38AM +0100, Arnaud Fontaine wrote:
> >>>>> "Jamey" == Jamey Sharp <jamey at minilop.net> writes:
>     >  In   xcb_ewmh_send_client_message,  you  probably   should  check
>     > data_len before copying  data into a fixed-size buffer--unless you
>     >  want to  declare that  function static  and ensure  that  all the
>     > callers  are safe. If  you want to  go the latter route  you might
>     > look at the BUILD_BUG_ON macro from include/linux/kernel.h, to get
>     > a compile-time check.
> 
> When adding the following macro:
> #define BUILD_BUG_OR_ZERO(e) ((void) (sizeof(struct { int:-!!(e); })))
> 
> And the following line before the memcpy:
> BUILD_BUG_OR_ZERO((data_len >> 2) > 5);
> 
> I get the following error I can't get rid of:
> ewmh.c:563: error: bit-field ‘<anonymous>’ width not an integer constant

Yeah, the data_len parameter in xcb_ewmh_send_client_message isn't a
constant, so it can't be checked at compile time. I meant that you'd add
something like this in every caller:

BUILD_BUG_ON(sizeof data > sizeof ((xcb_client_message_event_t *) 0)->data);

Unlike an assertion, this doesn't generate any code; instead the
compiler can prove that all those calls are safe, but only if you've
checked all the callers, which is why I suggested making this function
static. Also, you could wrap xcb_ewmh_send_client_message up in a macro
that does this, if you wanted.

> I'm  a bit  confused  by this  macro  and does  not understand  anything
> besides of  the basic principle. I  was thinking about  simply adding an
> assertion otherwise.

Absolutely, that would be better than nothing.

>     >  Huh. So  an  xcb_ewmh_connection_t  is only  usable  on a  single
>     > screen, and you have to  pick which one to use when you initialize
>     > it? I  know X's concept of screens is  becoming pretty obsolete in
>     > the RandR era, but if anything still has to deal with multi-screen
>     > setups it'd be window managers.
> 
> Actually, it's  the caller who takes care  of xcb_ewmh_connection_t. For
> instance, a window manager managing two displays would simply initialize
> xcb_ewmh_connection_t by calling xcb_ewmh_init_atoms() twice. I think it
> can't be done otherwise, because the Atoms are specific to a display and
> so  do  xcb_connection_t.  That's  why  xcb_ewmh_connection_t  has  been
> defined,  only to avoid  extra parameters  such as  xcb_connection_t and
> EWMH Atoms in each function of the API.

I'm not talking about different connections. I'm talking about different
screens on the same connection. All the atoms are guaranteed to be the
same in that case, except for _NET_WM_CM_Sn.

>     >  I'm a  little dismayed  that xcb_ewmh_init_atoms_replies  gets 82
>     > copies  of four nearly-identical lines  of code ...
> 
> Hope  that's not  too  ugly[1]  ;)

The patch looks good to me, except that I think it's wrong. :-) You're
assigning an XID to a dereferenced pointer of type "char", as far as I
can tell. I think you need one more cast on the left-hand side,
something like this:

*(xcb_atom_t *) ((char *) ewmh + ewmh_atoms[i].m_offset) = reply->atom;

Assuming I'm right, I'd think this bug would be pretty obvious, since at
least in my setup the X server looks like it's allocating more than 256
atoms before the window manager starts.

> That's why I have only defined accessors for _NET_WM_SYNC_REQUEST_COUNTER.

Yes, all your functions are named *_request_counter, but they all use
ewmh->_NET_WM_SYNC_REQUEST. Isn't that wrong?

And the comment header still has that typo of missing a "T". ;-)

>     >  Do you  really want  it in  the xcb-util  bundle though?
> 
> Well, I think  it's better to keep it within  xcb-util.

I'd be interested in hearing your thoughts on the discussion Julien and
I had today on this topic.

> For instance, if all the libraries provided in xcb-util  would have
> not been shipped as a bundle, I think they would be  in a much worst
> shape, mainly because the original author of  the code is rarely the
> maintainer  on a long stretch of time. Keeping it as a bundle ensures
> that even if the author does not have time anymore to maintain his
> code, someone else will take over the maintenance and  update it  when
> necessary (this  is what  happened with xcb-util/icccm at  least).

I think the history of free software contradicts this. For example,
bundling lots of code into libX11 certainly didn't help it be maintained
any better. :-) Conversely, lots of projects abandoned by their original
authors have been picked up by somebody else who needed them, despite
not being bundled with anything else.

> Moreover, xcb-util/icccm  and xcb-util/image are  already   used  in
> other  projects,  but   are  still   parts  of xcb-util.

Yes, so are aux, and render-util I think, and awesome is using the
property and event libraries apparently. And yes, I'd like all of those
split out, but just because that hasn't happened doesn't mean we should
add anything more to the mess.

Jamey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
Url : http://lists.freedesktop.org/archives/xcb/attachments/20091214/30dda590/attachment.pgp