Re: [Xcb] pthread stubs in libX11 vs. libxcb

[Uli Schlachter]

Hi,

Am 29.09.22 um 06:27 schrieb Keith Packard:

Alan Coopersmith  writes:


Does anyone disagree?


Yeah, synchronizing with xcb seems 'obviously right. The only question
I've got is that in 2022, should xcb ever use stubs for the thread functions?


No, it shouldn't. That's why since 2017, pthread-stub just links against 
pthread. It only does not do that if libc already includes the necessary 
stubs. See 
https://gitlab.freedesktop.org/xorg/lib/pthread-stubs/-/commit/8340ebd656c7e1a5b6d31170c1f3c87df043e793


Cheers,
Uli
--
"Do you know that books smell like nutmeg or some spice from a foreign 
land?"

  -- Faber in Fahrenheit 451

Re: [Xcb] pthread stubs in libX11 vs. libxcb

[Uli Schlachter]

Hi,

Am 29.09.22 um 02:29 schrieb Alan Coopersmith:
[...]
Which in hindsight, sounds a lot like that bug 162 linked above - the 
libX11
xlib_ctor would call XInitThreads() at library load time, which would 
use the
stubs to fake initializing the mutexes, and then libxcb would be loaded 
later
(likely when XOpenDisplay() is called) pulling in the real pthreads 
library,


Why is libxcb only loaded later? I would expect that by the time 
XInitThreads() can be called, the dynamic linker has at least loaded all 
dependent libraries. So, if libxcb links against libpthread, then 
libpthread should already be loaded at this time.


Is this thinking wrong?

Cheers,
Uli
--
"Do you know that books smell like nutmeg or some spice from a foreign 
land?"

  -- Faber in Fahrenheit 451

Re: [Xcb] [PATCH xcb] don't flag extra reply in xcb_take_socket

[Uli Schlachter]

On 14.08.2018 14:46, Julien Cristau wrote:
> +xcb@

Thanks, Julien.

And sorry for this mail getting stuck in xorg-devel's moderation queue,
I'll remove that CC if I reply again.

> On 08/09/2018 12:20 AM, Erik Kurzinger wrote:
>> If any flags are specified in a call to xcb_take_socket,
>> they should only be applied to replies for requests sent
>> after that function returns (and until the socket is
>> re-acquired by XCB).
>>
>> Previously, they would also be incorrectly applied to the
>> reply for the last request sent before the socket was taken.
>> For instance, in this example program the reply for the
>> GetInputFocus request gets discarded, even though it was
>> sent before the socket was taken. This results in the
>> call to retrieve the reply hanging indefinitely.

Thanks for figuring this out. Still, I'm slightly confused about this. I
added another GetInputFocus request after the xcb_take_socket(). If I
get the reply for this second request first, everything works fine. If I
get the reply for this second request second, getting the first reply
still hangs. (See attached file)

I fail to understand this behaviour. Since pending replies are applied
when receiving responses from the server, shouldn't the order that I
actually fetch the replies from XCB make no difference?

(Also, I patched my local xcb with just your change to
xcb_take_socket(), expecting this to cause the first request after
taking the socket to be discarded, but this did not happen either)

I feel like I am understanding less than before I started to figure this
out...

Cheers,
Uli

[...]
>> diff --git a/src/xcb_out.c b/src/xcb_out.c
>> index 3601a5f..c9593e5 100644
>> --- a/src/xcb_out.c
>> +++ b/src/xcb_out.c
>> @@ -387,8 +387,14 @@ int xcb_take_socket(xcb_connection_t *c, void 
>> (*return_socket)(void *closure), v
>>  {
>>  c->out.return_socket = return_socket;
>>  c->out.socket_closure = closure;
>> -if(flags)
>> -_xcb_in_expect_reply(c, c->out.request, 
>> WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
>> +if(flags) {
>> +/* c->out.request + 1 will be the first request sent by the 
>> external
>> + * socket owner. If the socket is returned before this request 
>> is sent
>> + * it will be detected in _xcb_in_replies_done and this 
>> pending_reply
>> + * will be discarded.
>> + */
>> +_xcb_in_expect_reply(c, c->out.request + 1, 
>> WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
>> +}
>>  assert(c->out.request == c->out.request_written);
>>  *sent = c->out.request;
>>  }
>>
-- 
- Buck, when, exactly, did you lose your mind?
- Three months ago. I woke up one morning married to a pineapple.
  An ugly pineapple... But I loved her.
#include 
#include 
#include 

static void return_socket(void *closure) {
	(void) closure;
}

int main(void)
{
xcb_connection_t *c = xcb_connect(NULL, NULL);

xcb_get_input_focus_cookie_t cookie1 = xcb_get_input_focus_unchecked(c);

uint64_t seq;
xcb_take_socket(c, return_socket, NULL, XCB_REQUEST_DISCARD_REPLY, &seq);

xcb_get_input_focus_cookie_t cookie2 = xcb_get_input_focus_unchecked(c);

xcb_generic_error_t *err;
#if 0
// This version does not hang
puts("before 2");
xcb_get_input_focus_reply(c, cookie2, &err);
puts("before 1");
xcb_get_input_focus_reply(c, cookie1, &err);
#else
// This version hangs
puts("before 1");
xcb_get_input_focus_reply(c, cookie1, &err);
puts("before 2");
xcb_get_input_focus_reply(c, cookie2, &err);
#endif
puts("done");
}
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [Xcb] [PATCH] Introduce xcb_aux_alloc_shm() to create anonymous files in RAM

[Uli Schlachter]

On 10.04.2018 13:38, Alexander Volkov wrote:
> ... which then can be mapped with mmap().
> It is intended to be used in conjunction with xcb_shm_attach_fd()
> to access the created shared memory from a client and the X server.
> 
> Signed-off-by: Alexander Volkov 
> ---
>  configure.ac  | 47 +
>  src/xcb_aux.c | 83 +++
>  src/xcb_aux.h |  3 ++
>  3 files changed, 133 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 1fe1561..81e1f6b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -7,10 +7,57 @@ AC_CONFIG_HEADERS([config.h])
>  AC_CONFIG_MACRO_DIR([m4])
>  AM_INIT_AUTOMAKE([foreign dist-bzip2])
>  
> +AC_USE_SYSTEM_EXTENSIONS
> +
>  XCB_UTIL_COMMON([1.4], [1.6])
>  
>  AC_CHECK_FUNCS_ONCE(vasprintf)
>  
> +AC_CHECK_FUNCS(memfd_create mkostemp)
> +
> +AC_CHECK_DECLS([__NR_memfd_create], [], [], [[#include ]])
> +
> +AC_CHECK_HEADERS([sys/memfd.h], [AC_DEFINE([HAVE_MEMFD_H], 1, [Has 
> sys/memfd.h header])])
> +
> +AC_ARG_WITH(shared-memory-dir, 
> AS_HELP_STRING([--with-shared-memory-dir=PATH], [Path to directory in a 
> world-writable temporary directory for anonymous shared memory (default: 
> auto)]),

Default is yes, not auto. See next two lines.

> +[],
> +[with_shared_memory_dir=yes])
> +
> +shmdirs="/run/shm /dev/shm /var/tmp /tmp"
> +
> +case x"$with_shared_memory_dir" in
> +xyes)
> + for dir in $shmdirs; do
> + case x"$with_shared_memory_dir" in
> + xyes)

How many shells are there that do not like "break"? :-/
(And does autoconf provide some way to make this loop nicer?)

> + echo Checking temp dir "$dir"
> + if test -d "$dir"; then
> + with_shared_memory_dir="$dir"
> + fi
> + ;;
> + esac
> + done
> + ;;
> +x/*)
> + ;;
> +xno)
> + ;;
> +*)
> + AC_MSG_ERROR([Invalid directory specified for --with-shared-memory-dir: 
> $with_shared_memory_dir])
> + ;;
> +esac
> +
> +case x"$with_shared_memory_dir" in
> +xyes)
> + AC_MSG_ERROR([No directory found for shared memory temp files.])
> + ;;
> +xno)
> + ;;
> +*)
> + AC_DEFINE_UNQUOTED(SHMDIR, ["$with_shared_memory_dir"], [Directory for 
> shared memory temp files])
> + ;;
> +esac
> +
>  AC_CONFIG_FILES([Makefile
>   src/Makefile
>   xcb-atom.pc

So, if I do --with-shared-memory-dir=/foobar, configure will tell me
that I specified an invalid directory? Why?? And why does this flag
exist at all, i.e. what would be uses for it?

Also, why does the existence of a directory at compile time have any
significance at run time? What about cross compiling? Building a distro
package with /run/shm existing and then running it on another system
that does not have /run/shm?

I really think that this compile time check is plain wrong.

Also, is it really worth it to have all these different cases?

- Linux with updated glibc that provides a memfd_create() wrapper
- Linux with older glibc where this code provides its own wrapper
- Older Linux with memfd_create() (-> mkostemp())
- Anything else (-> mkstemp())

How about dropping the second one (own syscall wrapper if glibc does not
provide one). Oh and how about making sys/memfd.h required instead of
providing the defines here instead, if needed.

What would be the downside of this? How many people would complain?

> diff --git a/src/xcb_aux.c b/src/xcb_aux.c
> index b6f64f8..e8f7ba9 100644
> --- a/src/xcb_aux.c
> +++ b/src/xcb_aux.c
> @@ -33,9 +33,39 @@
>  #include "config.h"
>  #endif
>  
> +#if !HAVE_MEMFD_CREATE
> +#if HAVE_DECL___NR_MEMFD_CREATE
> +#include 
> +#include 
> +static int memfd_create(const char *name,
> + unsigned int flags)
> +{
> + return syscall(__NR_memfd_create, name, flags);
> +}
> +#define HAVE_MEMFD_CREATE1
> +#endif
> +#endif
> +
> +#if HAVE_MEMFD_CREATE
> +
> +/* Get defines for the memfd_create syscall, using the
> + * header if available, or just defining the constants otherwise
> + */
> +
> +#if HAVE_MEMFD_H
> +#include 
> +#else
> +/* flags for memfd_create(2) (unsigned int) */
> +#define MFD_CLOEXEC  0x0001U
> +#define MFD_ALLOW_SEALING0x0002U
> +#endif

This looks quite linux-specific. Is it worth adding a linux-specific #if
in here in case any other platform ever implements this API as well and
assigns flags differently?

Also, why MFD_ALLOW_SEALING?

[...]
> +int
> +xcb_aux_alloc_shm(size_t size)

Should the documentation mention anything about sealing? close-on-exec?

> +{
> + chartemplate[] = SHMDIR "/shmfd-XX";

This uses shmfd, but...

> + int fd;
> +
> +#if HAVE_MEMFD_CREATE
> + fd = memfd_create("xcb_aux", MFD_CLOEXEC|MFD_ALLOW_SEALING);
[...]

...this uses xcb_aux.

Shouldn't they best use the same template name?

Cheers,
Uli
-- 
Happiness can't be found -- it finds you.
 - Majic
_

Re: [Xcb] [PATCH xcb/proto] screensaver: Use CARD32 encoding for ScreenSaverSuspend 'suspend'

[Uli Schlachter]

Ah, that explains why Keith pushed a patch on 14th that was never on the
mailing list. (= This patch is already merged.)

Oh and while we are at it:
I cannot find the exact definition of the struct that the X server uses
for this, but it definitely only compares the "suspend" member of the
incoming request against "TRUE" and "FALSE". Presumably, these have
values 0 and 1. I see two problems with this:

a) SProcScreenSaverSuspend() only swaps the request length, not the boolean
b) What happens if I just send directly a value of 42?

For (b), it seems like the behaviour depends on whether the sending
client already suspended the screen saver before or not. In the first
case, 42 is interpreted as FALSE while in the second as TRUE.

Cheers,
Uli

On 20.03.2018 20:56, Julien Cristau wrote:
> I think this belongs on xcb@.
> 
> Cheers,
> Julien
> 
> On Mon, Mar 12, 2018 at 12:03:19 -0700, Keith Packard wrote:
> 
>> This was set to BOOL, but the protocol headers used Bool, presumably a
>> 32-bit type. We're switching everything to CARD32 as the best option
>> for compatibility.
>>
>> Signed-off-by: Keith Packard 
>> ---
>>  src/screensaver.xml | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/src/screensaver.xml b/src/screensaver.xml
>> index 8d5abb4..c546f94 100644
>> --- a/src/screensaver.xml
>> +++ b/src/screensaver.xml
>> @@ -168,8 +168,7 @@ Draft Standard Version 1.1
>>
>>
>>
>> -
>> -
>> +
>>
>>  
>>
>> -- 
>> 2.16.2
>>
>> ___
>> xorg-devel@lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> ___
> Xcb mailing list
> x...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/xcb
> 


-- 
99 little bugs in the code
99 little bugs in the code
Take one down, patch it around
117 little bugs in the code
  -- @irqed
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [Xcb] [PATCH xcb] Fix hanging issue in _XReply

[Uli Schlachter]

[Keeping CC list, sorry for this ending up in the moderation queue of
xorg-devel]

On 14.02.2018 22:38, Adam Jackson wrote:
> On Fri, 2018-01-05 at 16:49 +0200, Ian Ray wrote:
>> From: Tatu Frisk 
>>
>> Assume event queue is empty if another thread is blocking waiting for event.
>>
>> If one thread was blocking waiting for an event and another thread sent a
>> reply to the X server, both threads got blocked until an event was
>> received.
>>
>> Signed-off-by: Tatu Frisk 
>> Signed-off-by: Jose Alarcon 
>> Signed-off-by: Ian Ray 
> 
> I came across this patch again today. It looks correct to me but I
> don't claim to fully understand libX11; it would be nice if an xcb
> expert could take a look.

I would say that anyone who claims to fully understand libX11 must be
wrong. ;-)

>>  src/xcb_io.c | 18 +++---
>>  1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/xcb_io.c b/src/xcb_io.c
>> index 649c820..4a8bb27 100644
>> --- a/src/xcb_io.c
>> +++ b/src/xcb_io.c
>> @@ -607,18 +607,14 @@ Status _XReply(Display *dpy, xReply *rep, int extra, 
>> Bool discard)
>>  if(dpy->xcb->event_owner == XlibOwnsEventQueue)
>>  {
>>  xcb_generic_reply_t *event;
>> -/* If some thread is already waiting for events,
>> - * it will get the first one. That thread must
>> - * process that event before we can continue. */
>> -/* FIXME: That event might be after this reply,
>> - * and might never even come--or there might be
>> - * multiple threads trying to get events. */
>> -while(dpy->xcb->event_waiter)
>> -{ /* need braces around ConditionWait */
>> -ConditionWait(dpy, dpy->xcb->event_notify);
>> +
>> +/* Assume event queue is empty if another thread is 
>> blocking
>> + * waiting for event. */
>> +if(!dpy->xcb->event_waiter)
>> +{
>> +while((event = poll_for_response(dpy)))
>> +handle_response(dpy, event, True);
>>  }
>> -while((event = poll_for_event(dpy)))
>> -handle_response(dpy, event, True);
>>  }
>>  
>>  req->reply_waiter = 0;

I would also say that this is more of a libX11 question and an xcb one.
Anyway...

So, this patch assumes that dpy->xcb_event_waiter != 0 means that there
cannot be any events in XCB's(?) event queue. When is event_waiter set?
The only place I can find is in libX11:src/xcb_io.c, function
_XReadEvents(), currently lines 382 to 402 which is (simplified pseudo
code):

if(!dpy->xcb->next_event)
{
event_waiter = 1;
UnlockDisplay(dpy);
xcb_wait_for_event();
InternalLockDisplay(dpy, 1);
event_waiter = 0;
dpy->xcb->next_event = 0;
}

This code clearly does not do anything to empty the event queue. Let's
just assume that there is an event sitting in xcb's event queue, then
event_waiter = 1 is set, the display is unlocked and other threads are
free to observe this even though an event is waiting.

So to me, this patch looks wrong.

To produce a correct patch, I have to wonder: What is *the* event queue?
Is it events queued in xcb, or also events queued for receiving on the
socket?

If this really just means events queued in libxcb, then _XReadEvents()
has to check xcb_poll_for_queued_event() *without* setting event_waiter
or unlocking the display. Only if this returns NULL can it do what it
currently does. This NULL clearly means that (at some point) the event
queue was empty.

If the socket has to be polled, the right function to use is
xcb_poll_for_event() and the same argumentation as above applies.

However, I am not sure that the above is correct, since
there is still a race when some event arrives: If another thread
currently has the display locked, it could see the assumption/invariant
"all events that the X11 server sent before it sent the reply that I am
handling right now were already handled" violated.

I do not know enough about libX11 to know the precise wording of this
invariant, nor do I know why it is needed.

Another thought: The problem seems to be that libX11 wants to handle all
"packets" in on-the-wire order, but libxcb does not directly offer a way
to do this. So, sometimes it has to call xcb_wait_for_event() even
though it cannot be sure that one arrives (right?). If so, couldn't
poll()ing on the socket be used to wait until "something" happens and
then xcb_poll_for_event() / xcb_poll_for_reply() can be used to figure
out what happened?
I'm not sure and I do not really know libX11.

Cheers,
Uli
-- 
A learning experience is one of those things that say,
'You know that thing you just did? Don't do that.'
 -- Douglas Adams
__

Re: [PATCH xserver] composite: Fix repaint of borders (v2)

[Uli Schlachter]

On 06.12.2016 20:11, Adam Jackson wrote:
[...]
> @@ -126,19 +127,20 @@ compSetPixmapVisitWindow(WindowPtr pWindow, void *data)
>   */
>  SetWinSize(pWindow);
>  SetBorderSize(pWindow);
> -if (HasBorder(pWindow))
> +if (pVisit->bw)
>  QueueWorkProc(compRepaintBorder, serverClient,
>(void *) (intptr_t) pWindow->drawable.id);
>  return WT_WALKCHILDREN;
>  }
[...]

Sorry, but I think that this is wrong. The definition of
HasBorder(pWindow) is in include/windowstr.h:

#define HasBorder(w)   ((w)->borderWidth || wClipShape(w))

Your patch drops the shape check.

Cheers,
Uli
-- 
Bruce Schneier can read and understand Perl programs.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver] composite: Fix repaint of borders

[Uli Schlachter]

When going from border width zero to a non-zero border width, the
composite extension is informed via the ConfigNotify callback. The
call-chain looks like this: compConfigNotify -> compReallocPixmap ->
compSetPixmap -> TraverseTree -> compSetPixmapVisitWindow. However, at
this time, pWindow->borderWidth was not yet updated. Thus, HasBorder()
is false and the window border will not be repainted.

Fix this by unconditionally queuing the call to compRepaintBorder().
This function is then enhanced to check if a border exists.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98499
Signed-off-by: Uli Schlachter 
---
Please keep me CC'd to replies. Also, feel free to come up with
other/better fixes for this. :-)

 composite/compwindow.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/composite/compwindow.c b/composite/compwindow.c
index 344138a..3323df6 100644
--- a/composite/compwindow.c
+++ b/composite/compwindow.c
@@ -99,7 +99,7 @@ compRepaintBorder(ClientPtr pClient, void *closure)
 dixLookupWindow(&pWindow, (XID) (intptr_t) closure, pClient,
 DixWriteAccess);
 
-if (rc == Success) {
+if (rc == Success && HasBorder(pWindow)) {
 RegionRec exposed;
 
 RegionNull(&exposed);
@@ -126,9 +126,8 @@ compSetPixmapVisitWindow(WindowPtr pWindow, void *data)
  */
 SetWinSize(pWindow);
 SetBorderSize(pWindow);
-if (HasBorder(pWindow))
-QueueWorkProc(compRepaintBorder, serverClient,
-  (void *) (intptr_t) pWindow->drawable.id);
+QueueWorkProc(compRepaintBorder, serverClient,
+  (void *) (intptr_t) pWindow->drawable.id);
 return WT_WALKCHILDREN;
 }
 
-- 
2.10.2

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2 xserver] EXA: Honour op parameter to exaGlyphs even if maskFormat == NULL

[Uli Schlachter]

Am 06.05.2016 um 08:47 schrieb Michel Dänzer:
> On 05.05.2016 22:30, Uli Schlachter wrote:
>> Am 01.04.2016 um 11:24 schrieb Michel Dänzer:
>>> From: Michel Dänzer 
>>>
>>> Reported-by: Uli Schlachter 
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94775
>>> Signed-off-by: Michel Dänzer 
>>
>> Ping? Does anyone with lots of clue about EXA want to take a look at this?
>>
>> If it helps (since it's a step in the right direction and it becomes harder 
>> to
>> get wrong rendering after this patch):
> 
> Assuming "wrong rendering" refers to this paragraph from the referenced
> bug you reported:
> 
>  However, that could produce incorrect rendering with intermediate
>  flushes and some operators that cannot easily split (e.g. Out).
> 
> That would be a client bug, the RENDER specification explicitly defines
> how rendering is performed when mask-format is None.

I'm sorry. You are right. I wrongly remembered that the spec always required to
'Add' the glyphs to a temporary surface. In this case the patch really fixes all
the issues that I can think of. My R-b still stands. :-)

Uli
-- 
Bruce Schneier can read and understand Perl programs.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2 xserver] EXA: Honour op parameter to exaGlyphs even if maskFormat == NULL

[Uli Schlachter]

Am 01.04.2016 um 11:24 schrieb Michel Dänzer:
> From: Michel Dänzer 
> 
> Reported-by: Uli Schlachter 
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94775
> Signed-off-by: Michel Dänzer 

Ping? Does anyone with lots of clue about EXA want to take a look at this?

If it helps (since it's a step in the right direction and it becomes harder to
get wrong rendering after this patch):

Reviewed-by: Uli Schlachter 

> ---
> 
> v2: Add proper Reported-by: and Bugzilla: tags.
> 
>  exa/exa_glyphs.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/exa/exa_glyphs.c b/exa/exa_glyphs.c
> index cf21ea9..192a643 100644
> --- a/exa/exa_glyphs.c
> +++ b/exa/exa_glyphs.c
> @@ -618,9 +618,9 @@ exaGlyphsToMask(PicturePtr pMask, ExaGlyphBufferPtr 
> buffer)
>  }
>  
>  static void
> -exaGlyphsToDst(PicturePtr pSrc, PicturePtr pDst, ExaGlyphBufferPtr buffer)
> +exaGlyphsToDst(CARD8 op, PicturePtr pSrc, PicturePtr pDst, ExaGlyphBufferPtr 
> buffer)
>  {
> -exaCompositeRects(PictOpOver, pSrc, buffer->mask, pDst, buffer->count,
> +exaCompositeRects(op, pSrc, buffer->mask, pDst, buffer->count,
>buffer->rects);
>  
>  buffer->count = 0;
> @@ -801,7 +801,7 @@ exaGlyphs(CARD8 op,
> 0, 0, x - glyph->info.x,
> y - glyph->info.y)
>  == ExaGlyphNeedFlush) {
> -exaGlyphsToDst(pSrc, pDst, &buffer);
> +exaGlyphsToDst(op, pSrc, pDst, &buffer);
>  exaBufferGlyph(pScreen, &buffer, glyph, pSrc, pDst,
> xSrc + (x - glyph->info.x) - 
> first_xOff,
> ySrc + (y - glyph->info.y) - 
> first_yOff,
> @@ -821,7 +821,7 @@ exaGlyphs(CARD8 op,
>  if (maskFormat)
>  exaGlyphsToMask(pMask, &buffer);
>  else
> -exaGlyphsToDst(pSrc, pDst, &buffer);
> +exaGlyphsToDst(op, pSrc, pDst, &buffer);
>  }
>  
>  if (maskFormat) {
> 


-- 
Homophobia - The fear that another man will treat you the way you treat women.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH inputproto] specs: Set TZ=UTC before calling asciidoc

[Uli Schlachter]

Am 10.12.2015 um 21:25 schrieb Andreas Boll:
> Set TZ=UTC before calling asciidoc to make the embedded dates invariant
> to timezones in order to make the package build reproducibly.
> 
> Fixes bug:
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=795981
> 
> Suggested-by: Eduard Sanou 
> Signed-off-by: Andreas Boll 
> ---
>  specs/Makefile.am | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/specs/Makefile.am b/specs/Makefile.am
> index a83cf40..d04f4d0 100644
> --- a/specs/Makefile.am
> +++ b/specs/Makefile.am
> @@ -6,7 +6,7 @@ doc_DATA = XI2proto.html XIproto.html
>  dist_doc_DATA = XI2proto.txt XIproto.txt
>  
>  %.html: %.txt
> - $(AM_V_GEN)$(ASCIIDOC) -o $@ $<
> + TZ=UTC $(AM_V_GEN)$(ASCIIDOC) -o $@ $<

Shouldn't this be $(AM_V_GEN)TZ=UTC $(ASCIIDOC) -o $@ $  
>  CLEANFILES = $(doc_DATA)
>  
> 

Uli
-- 
A normal person is just someone you don't know well enough yet.
 - Nettie Wiebe
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH RFC kdrive/ephyr v2] Match host X server's keymap

[Uli Schlachter]

Hi,

Am 08.12.2015 um 13:46 schrieb Laércio de Sousa:
[...]
> diff --git a/hw/kdrive/ephyr/hostx.c b/hw/kdrive/ephyr/hostx.c
> index 49516bb..249b210 100644
> --- a/hw/kdrive/ephyr/hostx.c
> +++ b/hw/kdrive/ephyr/hostx.c
[...]
> @@ -1086,18 +1085,105 @@ hostx_paint_debug_rect(KdScreenInfo *screen,
>  nanosleep(&tspec, NULL);
>  }
>  
> -void
> -hostx_load_keymap(void)
> +Bool
> +hostx_load_keymap(KeySymsPtr keySyms, CARD8 *modmap, XkbControlsPtr controls)
>  {
>  int min_keycode, max_keycode;
> +int map_width;
> +size_t i, j;
> +int keymap_len;
> +xcb_keysym_t *keymap;
> +xcb_keycode_t *modifier_map;
> +xcb_get_keyboard_mapping_cookie_t mapping_c;
> +xcb_get_keyboard_mapping_reply_t *mapping_r;
> +xcb_get_modifier_mapping_cookie_t modifier_c;
> +xcb_get_modifier_mapping_reply_t *modifier_r;
> +xcb_xkb_use_extension_cookie_t use_c;
> +xcb_xkb_use_extension_reply_t *use_r;
> +xcb_xkb_get_controls_cookie_t controls_c;
> +xcb_xkb_get_controls_reply_t *controls_r;
>  
>  min_keycode = xcb_get_setup(HostX.conn)->min_keycode;
>  max_keycode = xcb_get_setup(HostX.conn)->max_keycode;
>  
>  EPHYR_DBG("min: %d, max: %d", min_keycode, max_keycode);
>  
> -ephyrKeySyms.minKeyCode = min_keycode;
> -ephyrKeySyms.maxKeyCode = max_keycode;
> +keySyms->minKeyCode = min_keycode;
> +keySyms->maxKeyCode = max_keycode;
> +
> +use_c = xcb_xkb_use_extension(HostX.conn,
> +  XCB_XKB_MAJOR_VERSION,
> +  XCB_XKB_MINOR_VERSION);
> +use_r = xcb_xkb_use_extension_reply(HostX.conn, use_c, NULL);

Could you check xcb_get_extension_data(HostX.conn, &xcb_xkb_id)->present first?
I don't know why XKB's UseExtension has a supported field, but without this
suggested check, the XCB connection will just go in an error state if the XKB
connection is not actually present.

> +
> +if (!use_r) {
> +EPHYR_LOG_ERROR("Couldn't use XKB extension.");
> +return FALSE;
> +} else if (!use_r->supported) {
> +EPHYR_LOG_ERROR("XKB extension is not supported in X server.");
> +free(use_r);
> +return FALSE;
> +}
> +
> +free(use_r);
> +
> +controls_c = xcb_xkb_get_controls(HostX.conn,
> +  XCB_XKB_ID_USE_CORE_KBD);
> +controls_r = xcb_xkb_get_controls_reply(HostX.conn,
> +controls_c,
> +NULL);
> +
> +if (!controls_r) {
> +EPHYR_LOG_ERROR("Couldn't get XKB keyboard controls.");
> +return FALSE;

For this one you check for errors, with the following ones you do not. Is there
a reason for that?

> +}
> +
> +mapping_c = xcb_get_keyboard_mapping(HostX.conn,
> + min_keycode,
> + max_keycode - min_keycode + 1);
> +mapping_r = xcb_get_keyboard_mapping_reply(HostX.conn,
> +   mapping_c,
> +   NULL);
> +map_width = mapping_r->keysyms_per_keycode;
> +keymap = xcb_get_keyboard_mapping_keysyms(mapping_r);
> +keymap_len = xcb_get_keyboard_mapping_keysyms_length(mapping_r);
> +
> +modifier_c = xcb_get_modifier_mapping(HostX.conn);
> +modifier_r = xcb_get_modifier_mapping_reply(HostX.conn,
> +modifier_c,
> +NULL);

Also, even though it will never matter much, could you first send all the
requests and then get the replies?

[...]

No clue about the rest. I don't really understand this. :-)

Cheers,
Uli
-- 
“Some people are worth melting for.” - Olaf
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xlib] Add support for a display-specific error handler

[Uli Schlachter]

Am 16.11.2015 um 21:57 schrieb Jasper St. Pierre:
> Now do this:
> 
> static void Foo()
> {
> Request1();
> Request2();
> }
> 
> static void Bar()
> {
> Request1();
> Request3();
> }
> 
> static void Thing(int x)
> {
> SetErrorHandler();
> if (x == 0)
> Foo();
> else
> Bar();
> UnsetErrorHandlerAndCheck();
> }
> 
> We have a similar codepath in our driver and this is where I gave up
> and started on the Xlib solution.

I agree that this gets a little more complicated, but ok:

struct pending_checked_requests {
  struct pending_checked_requests *next;
 xcb_void_request_t request;
};

static void add_checked(struct pending_checked_requests **pend,
xcb_void_request_t request)
{
  struct pending_checked_requests *n = malloc(sizeof(*n));
  n->request = request;
  n->next = *pend;
  *pend = n;
}

static int check_errors(struct pending_checked_requests *pend)
{
  int result = 0;
  while (pend) {
if (xcb_request_check(pend->request)) {
   free error;
   result = 1;
}
struct pending_checked_requests *last = pend;
pend = pend->next;
free(last);
  }
  return result;
}

static void Bar(struct pcq **pend)
{
  add_checked(pend, Request1());
  add_checked(pend, Request2());
}

static int Thing(int x)
{
  struct pending_checked_requests *pend = NULL;
  if (x == 0)
Bar(&pend);
  return check_errors(pend);
}

(And if (which I guess) the frequent malloc()s are a problem, then add something
more sophisticated where you e.g. have space for 20 requests in each such
structure and use stack space for the 50 first requests.)

> On Mon, Nov 16, 2015 at 12:39 PM, Uli Schlachter  wrote:
>> Am 16.11.2015 um 18:06 schrieb Jasper St. Pierre:
>> [...]
>>> We currently use a behavior like:
>>>
>>> SetErrorHandler();
>>> Request1();
>>> Request2();
>>> Request3();
>>> if (UnsetErrorHandlerAndCheck())
>>> goto error;
>>>
>>> UnsetErrorHandlerAndCheck will XSync, and look at any newly incoming
>>> errors, check their serials so that they match up, and if they are
>>> inside those bounds, we return the error to the user and return that
>>> no error happened. Fairly standard trick.
>>>
>>> With xcb, I can do:
>>>
>>> if (check_request(request_1_checked(), &err) {
>>> free(err);
>>> goto error;
>>> }
>>> if (check_request(request_2_checked(), &err) {
>>> free(err);
>>> goto error;
>>> }
>>> if (check_request(request_3_checked(), &err) {
>>> free(err);
>>> goto error;
>>> }
>>>
>>> So that's clearly unusable.
>>
>> Well, why?
>>
>> How about this version:
>>
>> xcb_void_cookie_t cookies[3];
>>
>> cookies[0] = request_1_checked();
>> cookies[1] = request_2_checked();
>> cookies[2] = request_3_checked();
>>
>> bool had_error = false;
>> for (int i = 0; i < 3; i++) {
>>   xcb_generic_error_t *err = xcb_request_check(c, cookies[i]);
>>   had_error |= err != NULL;
>>   free(NULL);
>> }
>>
>> This only syncs once during the first call to xcb_request_check() and 
>> afterwards
>> it's done. This even improves parallelism, because you do not have to
>> XLockDisplay() to lock out other threads (which also means that "just a 
>> range of
>> sequence numbers" isn't enough and an array is needed).
>>
>> You could even generalize this with some helper functions to make it nicer,
>> similar to your SetErrorHandler() and UnsetErrorHandlerAndCheck() above.
>>
>> Uli
>>
>> [...]
>>> The Display-specific error handler solves my needs here by giving me a
>>> callback to that above model that's locked to that local section that
>>> works well from a library -- this code runs in a different thread, and
>>> we had issues where we overwrite another thread's error handler (in my
>>> case, the two threads were using different Displays, as they should).
>>>
>>> On Sun, Nov 15, 2015 at 11:56 PM, Keith Packard  wrote:
>>>> "Jasper St. Pierre"  writes:
>>>>
>>>>> Writing error-safe code that uses Xlib is too obnoxious, and using XCB
>>>>> is tedious and not performant, as we can't catch events on a giant
>>>>> stream -- we have to check every operation manually.
>>>>
>>>> You get errors returned in the event stream; is there something fancier
>>>> you'd like from xcb to make it faster?
>>>>
>>>> --
>>>> -keith
>>
>> --
>> "Every once in a while, declare peace. It confuses the hell out of your 
>> enemies"
>>  - 79th Rule of Acquisition
>> ___
>> xorg-devel@lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: http://lists.x.org/mailman/listinfo/xorg-devel
> 
> 
> 


-- 
Who needs a ~/.signature anyway?
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xlib] Add support for a display-specific error handler

[Uli Schlachter]

Am 16.11.2015 um 18:06 schrieb Jasper St. Pierre:
[...]
> We currently use a behavior like:
> 
> SetErrorHandler();
> Request1();
> Request2();
> Request3();
> if (UnsetErrorHandlerAndCheck())
> goto error;
> 
> UnsetErrorHandlerAndCheck will XSync, and look at any newly incoming
> errors, check their serials so that they match up, and if they are
> inside those bounds, we return the error to the user and return that
> no error happened. Fairly standard trick.
> 
> With xcb, I can do:
> 
> if (check_request(request_1_checked(), &err) {
> free(err);
> goto error;
> }
> if (check_request(request_2_checked(), &err) {
> free(err);
> goto error;
> }
> if (check_request(request_3_checked(), &err) {
> free(err);
> goto error;
> }
>
> So that's clearly unusable.

Well, why?

How about this version:

xcb_void_cookie_t cookies[3];

cookies[0] = request_1_checked();
cookies[1] = request_2_checked();
cookies[2] = request_3_checked();

bool had_error = false;
for (int i = 0; i < 3; i++) {
  xcb_generic_error_t *err = xcb_request_check(c, cookies[i]);
  had_error |= err != NULL;
  free(NULL);
}

This only syncs once during the first call to xcb_request_check() and afterwards
it's done. This even improves parallelism, because you do not have to
XLockDisplay() to lock out other threads (which also means that "just a range of
sequence numbers" isn't enough and an array is needed).

You could even generalize this with some helper functions to make it nicer,
similar to your SetErrorHandler() and UnsetErrorHandlerAndCheck() above.

Uli

[...]
> The Display-specific error handler solves my needs here by giving me a
> callback to that above model that's locked to that local section that
> works well from a library -- this code runs in a different thread, and
> we had issues where we overwrite another thread's error handler (in my
> case, the two threads were using different Displays, as they should).
>
> On Sun, Nov 15, 2015 at 11:56 PM, Keith Packard  wrote:
>> "Jasper St. Pierre"  writes:
>>
>>> Writing error-safe code that uses Xlib is too obnoxious, and using XCB
>>> is tedious and not performant, as we can't catch events on a giant
>>> stream -- we have to check every operation manually.
>>
>> You get errors returned in the event stream; is there something fancier
>> you'd like from xcb to make it faster?
>>
>> --
>> -keith

-- 
"Every once in a while, declare peace. It confuses the hell out of your enemies"
 - 79th Rule of Acquisition
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 10/20] kdrive/ephyr: Use NotifyFd for XCB connection input [v2]

[Uli Schlachter]

Am 13.11.2015 um 16:10 schrieb Keith Packard:
> Uli Schlachter  writes:
> 
>> If "something" uses xcb (even just sending a request is enough, doesn't have 
>> to
>> be waiting for a reply), xcb could read an event from its FD and append it to
>> its internal queue. At this point there is a pending event in the queue, but 
>> the
>> socket isn't marked readable. With this change, the event won't be removed 
>> from
>> the queue.
> 
> So, if we check for events before going to sleep (in the block handler),
> that should be sufficient to catch this I think?
> 

Yup.

If I understand the code correctly, that's exactly what the "old" code does.
-- 
Who needs a ~/.signature anyway?
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 10/20] kdrive/ephyr: Use NotifyFd for XCB connection input [v2]

[Uli Schlachter]

Am 12.11.2015 um 07:02 schrieb Keith Packard:
> Eliminates polling every 20ms for device input.
> 
> v2: rename ephyrPoll to ephyrXcbNotify and fix the API so it can be
> used directly for SetNotifyFd. Thanks to Daniel Martin
> 
> 
> Signed-off-by: Keith Packard 
> Cc: Daniel Martin 
> ---

Same comment as before: This can miss pending events.

If "something" uses xcb (even just sending a request is enough, doesn't have to
be waiting for a reply), xcb could read an event from its FD and append it to
its internal queue. At this point there is a pending event in the queue, but the
socket isn't marked readable. With this change, the event won't be removed from
the queue.

Cheers,

Uli

>  hw/kdrive/ephyr/ephyr.c | 6 --
>  hw/kdrive/ephyr/ephyr.h | 3 ---
>  hw/kdrive/ephyr/hostx.c | 6 ++
>  hw/kdrive/ephyr/hostx.h | 2 ++
>  hw/kdrive/ephyr/os.c| 1 -
>  5 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/kdrive/ephyr/ephyr.c b/hw/kdrive/ephyr/ephyr.c
> index cb1c16e..896bac5 100644
> --- a/hw/kdrive/ephyr/ephyr.c
> +++ b/hw/kdrive/ephyr/ephyr.c
> @@ -1182,8 +1182,8 @@ ephyrProcessConfigureNotify(xcb_generic_event_t *xev)
>  #endif /* RANDR */
>  }
>  
> -void
> -ephyrPoll(void)
> +static void
> +ephyrXcbNotify(int fd, int ready, void *data)
>  {
>  xcb_connection_t *conn = hostx_get_xcbconn();
>  
> @@ -1334,6 +1334,7 @@ static Status
>  MouseEnable(KdPointerInfo * pi)
>  {
>  ((EphyrPointerPrivate *) pi->driverPrivate)->enabled = TRUE;
> +SetNotifyFd(hostx_get_fd(), ephyrXcbNotify, X_NOTIFY_READ, NULL);
>  return Success;
>  }
>  
> @@ -1341,6 +1342,7 @@ static void
>  MouseDisable(KdPointerInfo * pi)
>  {
>  ((EphyrPointerPrivate *) pi->driverPrivate)->enabled = FALSE;
> +RemoveNotifyFd(hostx_get_fd());
>  return;
>  }
>  
> diff --git a/hw/kdrive/ephyr/ephyr.h b/hw/kdrive/ephyr/ephyr.h
> index 18bfe11..f5015f6 100644
> --- a/hw/kdrive/ephyr/ephyr.h
> +++ b/hw/kdrive/ephyr/ephyr.h
> @@ -168,9 +168,6 @@ Bool
>  Bool
>   ephyrCreateColormap(ColormapPtr pmap);
>  
> -void
> - ephyrPoll(void);
> -
>  #ifdef RANDR
>  Bool
>   ephyrRandRGetInfo(ScreenPtr pScreen, Rotation * rotations);
> diff --git a/hw/kdrive/ephyr/hostx.c b/hw/kdrive/ephyr/hostx.c
> index 3991c51..49516bb 100644
> --- a/hw/kdrive/ephyr/hostx.c
> +++ b/hw/kdrive/ephyr/hostx.c
> @@ -1113,6 +1113,12 @@ hostx_get_screen(void)
>  }
>  
>  int
> +hostx_get_fd(void)
> +{
> +return xcb_get_file_descriptor(HostX.conn);
> +}
> +
> +int
>  hostx_get_window(int a_screen_number)
>  {
>  EphyrScrPriv *scrpriv;
> diff --git a/hw/kdrive/ephyr/hostx.h b/hw/kdrive/ephyr/hostx.h
> index 9299e8d..d416dae 100644
> --- a/hw/kdrive/ephyr/hostx.h
> +++ b/hw/kdrive/ephyr/hostx.h
> @@ -198,4 +198,6 @@ int hostx_has_dri(void);
>  int hostx_has_glx(void);
>  #endif  /* XF86DRI */
>  
> +int hostx_get_fd(void);
> +
>  #endif /*_XLIBS_STUFF_H_*/
> diff --git a/hw/kdrive/ephyr/os.c b/hw/kdrive/ephyr/os.c
> index 0dbcbb8..b481d0a 100644
> --- a/hw/kdrive/ephyr/os.c
> +++ b/hw/kdrive/ephyr/os.c
> @@ -45,5 +45,4 @@ EphyrInit(void)
>  
>  KdOsFuncs EphyrOsFuncs = {
>  .Init = EphyrInit,
> -.pollEvents = ephyrPoll,
>  };
> 


-- 
- Buck, when, exactly, did you lose your mind?
- Three months ago. I woke up one morning married to a pineapple.
  An ugly pineapple... But I loved her.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-nested 09/10] Introduce a new XCB client backend, and make it the default one.

[Uli Schlachter]

Hi,

Am 06.11.2015 um 14:10 schrieb Laércio de Sousa:
[...]
>>> +static void
>>> +_NestedClientSetWMClass(NestedClientPrivatePtr pPriv,
>>> +const char* wm_class)
>>> +{
>>> +size_t class_len = strlen(wm_class) + 1;
>>> +char *class_hint = malloc(class_len);
>>> +
>>> +if (class_hint)
>>> +{
>>> +strcpy(class_hint, wm_class);
>>> +xcb_change_property(pPriv->conn,
>>> +XCB_PROP_MODE_REPLACE,
>>> +pPriv->window,
>>> +XCB_ATOM_WM_CLASS,
>>> +XCB_ATOM_STRING,
>>> +8,
>>> +class_len,
>>> +class_hint);
>>> +free(class_hint);
>>> +}
>>
>> Why is this strcpy needed?
>>
> I've copied over this piece of code from Xephyr. In that context,
> class_hint stores a concatenation bewteen two strings, so that strcpy is
> needed, but here the WM_CLASS string is much more simple, so that strcpy is
> not needed. I'll remove it. Thanks!

Uhm, right. This wants to set WM_CLASS and WM_CLASS should contain "two" strings
(the class and the instance), separated by a null byte.

Sorry for not noticing this earlier, but isn't this code wrong then?

https://tronche.com/gui/x/icccm/sec-4.html#s-4.1.2.5

[...]
>>> +if (!ev)
>>> +{
>>> +if (_NestedClientConnectionHasError(pPriv->scrnIndex,
>>> +pPriv->conn))
>>> +{
>>> +/* XXX: Is there a better way to do this? */
>>> +xf86DrvMsg(pPriv->scrnIndex,
>>> +   X_ERROR,
>>> +   "Connection with host X server lost.\n");
>>> +free(ev);
>>> +NestedClientCloseScreen(pPriv);
>>> +exit(1);
>>> +}
>>> +
>>> +break;
>>> +}
>>> +
>>> +switch (ev->response_type & ~0x80)
>>> +{
>>> +case XCB_EXPOSE:
>>> +_NestedClientProcessExpose(pPriv, ev);
>>> +break;
>>> +case XCB_CLIENT_MESSAGE:
>>> +_NestedClientProcessClientMessage(pPriv, ev);
>>> +break;
>>> +case XCB_MOTION_NOTIFY:
>>> +_NestedClientProcessMotionNotify(pPriv, ev);
>>> +break;
>>> +case XCB_KEY_PRESS:
>>> +_NestedClientProcessKeyPress(pPriv, ev);
>>> +break;
>>> +case XCB_KEY_RELEASE:
>>> +_NestedClientProcessKeyRelease(pPriv, ev);
>>> +break;
>>> +case XCB_BUTTON_PRESS:
>>> +_NestedClientProcessButtonPress(pPriv, ev);
>>> +break;
>>> +case XCB_BUTTON_RELEASE:
>>> +_NestedClientProcessButtonRelease(pPriv, ev);
>>> +break;
>>> +}
>>> +
>>> +free(ev);
>>> +xcb_flush(pPriv->conn);
>>
>> Why is this flushing inside of the event loop? Wouldn't a flush after the
>> loop
>> be enough?
>>
> Well... Comparing it with other XCB snippets I've found in the web, it
> seems this xcb_flush() call is not needed at all. Calling it right atfer
> window creation or redrawing (XCB_EXPOSE event only) should be enough,
> right?
[...]

Well, it depends. Something should flush in the end in case there are any
requests still in XCB's output buffer (and nothing implicitly flushes them by
waiting for a reply). So I think that having a call to xcb_flush() before
returning to this function should be added. If it does something, it just
prevented a bug and if it doesn't do anything, it's not expensive. ;-)

Everything else seems fine, thanks.

Uli
-- 
"For saving the Earth.. and eating cheesecake!"
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-nested 09/10] Introduce a new XCB client backend, and make it the default one.

[Uli Schlachter]

Am 05.11.2015 um 10:15 schrieb Laércio de Sousa:
> This patch brings up a new XCB backend client, translated from original
> xlibclient.c, and based on latest Xephyr code. This XCB backend
> brings some improvements already present in Xephyr, like
> fullscreen and output support.
> 
> This patch will also change configure.ac to make the new XCB
> backend the default one for building the driver. For switching back
> to Xlib backend, pass configure option --with-backend=xlib.
> 
> Signed-off-by: Laércio de Sousa 
> ---
[...]
> +else if (reply->major_version < major || reply->minor_version < minor)
> +{
> +xf86DrvMsg(scrnIndex,
> +   X_ERROR,
> +   "Host X server doesn't support RandR %d.%d, needed for 
> Option \"Output\" usage.\n",
> +   major, minor);

Why is 4.0 not better than 1.1? Shouldn't this check be replaced with the 
following?

  if (reply->major_version < major ||
 (reply->major_version == major && reply->minor_version < minor))

> +free(reply);
> +return FALSE;
> +}
> +
> +free(reply);
> +return TRUE;
> +}
[...]
> +static void
> +_NestedClientSetWMClass(NestedClientPrivatePtr pPriv,
> +const char* wm_class)
> +{
> +size_t class_len = strlen(wm_class) + 1;
> +char *class_hint = malloc(class_len);
> +
> +if (class_hint)
> +{
> +strcpy(class_hint, wm_class);
> +xcb_change_property(pPriv->conn,
> +XCB_PROP_MODE_REPLACE,
> +pPriv->window,
> +XCB_ATOM_WM_CLASS,
> +XCB_ATOM_STRING,
> +8,
> +class_len,
> +class_hint);
> +free(class_hint);
> +}

Why is this strcpy needed?

> +}
> +
[...]
> +static void
> +_NestedClientCreateWindow(NestedClientPrivatePtr pPriv)
> +{
> +xcb_size_hints_t sizeHints;
> +
> +sizeHints.flags = XCB_ICCCM_SIZE_HINT_P_POSITION
> +  | XCB_ICCCM_SIZE_HINT_P_SIZE
> +  | XCB_ICCCM_SIZE_HINT_P_MIN_SIZE
> +  | XCB_ICCCM_SIZE_HINT_P_MAX_SIZE;
> +sizeHints.min_width = pPriv->width;
> +sizeHints.max_width = pPriv->width;
> +sizeHints.min_height = pPriv->height;
> +sizeHints.max_height = pPriv->height;
> +
> +pPriv->window = xcb_generate_id(pPriv->conn);
> +pPriv->img = NULL;
> +
> +xcb_create_window(pPriv->conn,
> +  XCB_COPY_FROM_PARENT,
> +  pPriv->window,
> +  pPriv->rootWindow,
> +  0, 0, 100, 100, /* will resize */

Why not creating it at the correct size and position and skip at least the first
of the following two xcb_configure_window()? (Although I don't understand the
second one either)

> +  0,
> +  XCB_WINDOW_CLASS_COPY_FROM_PARENT,
> +  pPriv->visual->visual_id,
> +  pPriv->attr_mask,
> +  pPriv->attrs);
> +
> +xcb_icccm_set_wm_normal_hints(pPriv->conn,
> +  pPriv->window,
> +  &sizeHints);
> +
> +if (pPriv->usingFullscreen)
> +_NestedClientSetFullscreenHint(pPriv);
> +
> +_NestedClientSetDeleteWindowHint(pPriv);
> +_NestedClientSetWindowTitle(pPriv, "");
> +_NestedClientSetWMClass(pPriv, "Xorg");
> +
> +{
> +uint32_t mask = XCB_CONFIG_WINDOW_WIDTH | XCB_CONFIG_WINDOW_HEIGHT;
> +uint32_t values[2] = { pPriv->width, pPriv->height };
> +xcb_configure_window(pPriv->conn, pPriv->window, mask, values);
> +}
> +
> +xcb_map_window(pPriv->conn, pPriv->window);
> +
> +{
> +uint32_t mask = XCB_CONFIG_WINDOW_X | XCB_CONFIG_WINDOW_Y;
> +uint32_t values[2] = { pPriv->x, pPriv->y };
> +xcb_configure_window(pPriv->conn, pPriv->window, mask, values);
> +}
> +}
> +
[...]
> +void
> +NestedClientUpdateScreen(NestedClientPrivatePtr pPriv,
> + int16_t x1, int16_t y1,
> + int16_t x2, int16_t y2)
> +{
> +if (pPriv->usingShm)
> +xcb_image_shm_put(pPriv->conn, pPriv->window,
> +  pPriv->gc, pPriv->img,
> +  pPriv->shminfo,
> +  x1, y1, x1, y1, x2 - x1, y2 - y1, FALSE);
> +else
> +xcb_image_put(pPriv->conn, pPriv->window, pPriv->gc,
> +  pPriv->img, x1, y1, 0);
> +
> +xcb_aux_sync(pPriv->conn);
> +}

Does this really need a sync? Wouldn't a flush be enough? Or is the sync only
needed in the usingShm case so that the data isn't modified too early?

> +static inline void
> +_NestedClientProcessExpose(NestedClientPrivatePtr pPriv,
> +   xcb_generic_event_t *ev)
> +{
> +xcb_expose_event_t *xev = (xcb_expose_event_t *)ev;
> +Neste

Re: [PATCH xserver 15/24] kdrive/ephyr: Use NotifyFd for XCB connection input [v2]

[Uli Schlachter]

Hi,

Am 21.09.2015 um 11:32 schrieb Keith Packard:
> Daniel Martin  writes:
> 
> 
>> ephyrPoll() isn't used anywhere else. You could merge it into
>> kdrive_notify_conn().
> 
> Good call. I've renamed that to:
> 
> static void
> ephyrXcbNotify(int fd, int ready, void *data)
> 
> and eliminated the extra kdrive_notify_conn wrapper.

is this actually safe?

When you call any function reading from the XCB connection (e.g.
xcb_render_query_pict_formats_reply() in ephyrcursor.c, function
get_argb_format()), this can read an event from the underlying socket and buffer
this internally in XCB. So there would be an event available, but the FD is not
readable.

I guess you have to call ephyrXcbNotify() also before blocking.

Cheers,
Uli
-- 
Bruce Schneier can read and understand Perl programs.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11-xcb] add XCBGetXDisplay

[Uli Schlachter]

Hi,

Am 27.05.2015 um 14:42 schrieb Mike Blumenkrantz:
[...]
> From c5e4bf50a722670e0ac1b05509834874251c0c9c Mon Sep 17 00:00:00 2001
> From: Mike Blumenkrantz 
> Date: Wed, 27 May 2015 08:37:05 -0400
> Subject: [PATCH] add XCBGetXDisplay
> 
> a method for creating a Display object from an xcb connection

This commit message might be improved a bit.

> Signed-off-by: Mike Blumenkrantz 
> ---
>  include/X11/Xlib-xcb.h |   1 +
>  src/x11_xcb.c  | 562 
> +
>  2 files changed, 563 insertions(+)
> 
> diff --git a/include/X11/Xlib-xcb.h b/include/X11/Xlib-xcb.h
> index a21e2be..240a25c 100644
> --- a/include/X11/Xlib-xcb.h
> +++ b/include/X11/Xlib-xcb.h
> @@ -11,6 +11,7 @@
>  _XFUNCPROTOBEGIN
>  
>  xcb_connection_t *XGetXCBConnection(Display *dpy);
> +Display *XCBGetXDisplay(xcb_connection_t *c);
>  
>  enum XEventQueueOwner { XlibOwnsEventQueue = 0, XCBOwnsEventQueue };
>  void XSetEventQueueOwner(Display *dpy, enum XEventQueueOwner owner);
> diff --git a/src/x11_xcb.c b/src/x11_xcb.c
> index 3ddf403..0dabf6d 100644
> --- a/src/x11_xcb.c
> +++ b/src/x11_xcb.c
> @@ -1,9 +1,78 @@
[...]

Where did you copy these more than 500 lines from and why can't it be shared
with XOpenDisplay() (I guess that is where this code comes from)?

> + if ((display_name = getenv("DISPLAY")) == NULL) {
> + /* Oops! No DISPLAY environment variable - error. */
> + return(NULL);
> + }

Why does this function require $DISPLAY? That is totally unrelated to the XCB
connection that you give it.

Also, there is no documentation for this new function and I guess calling
XCloseDisplay() on it will also close the XCB connection. That doesn't seem 
right.

Cheers,
Uli
-- 
Happiness can't be found -- it finds you.
 - Majic
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-input-libinput 4/4] Add option "ButtonMapping" (#9206)

[Uli Schlachter]

Hi,

Am 29.04.2015 um 01:51 schrieb Peter Hutterer:
> With a long entry in the man page to detail what this option does.
> Specifically, it's the xorg.conf equivalent to XSetPointerMapping(3), it
> doesn't do any physical button remappings, merely the logical ones. If the
> physical button isn't mapped to the right logical button by default, that's
> either a libiput bug or an xkcd 1172 issue.
> 
> X.Org Bug 9206 
> 
> Signed-off-by: Peter Hutterer 
> ---
>  man/libinput.man | 38 +
>  src/libinput.c   | 57 
> +---
>  2 files changed, 88 insertions(+), 7 deletions(-)
> 
> diff --git a/man/libinput.man b/man/libinput.man
> index c5eaea5..3b2697e 100644
> --- a/man/libinput.man
> +++ b/man/libinput.man
[...]
> @@ -167,6 +180,31 @@ The above properties have a
>  .BI "libinput  Default"
>  equivalent that indicates the default value for this setting on this device.
>  
> +.SH BUTTON MAPPING
> +X clients receive events with logical button numbers, where 1, 2, 3
> +are usually interpreted as left, middle, right and logical buttons 4, 5, 6,
> +7 are usually interpreted as scroll up, down, left, right. The fourth and
> +fifth physical buttons on a device will thus send logical buttons 8 and 9.
> +The
> +.B ButtonMapping
> +option adjusts the logical button mapping, it does not affect how a physical
> +button is mapped to a logical button.
> +.PP
> +Traditionally, a device was set to left-handed button mode by applying a
> +button mapping of
> +.B "\*q3 2 1 ...\*q"
> +On systems using the
> +.B libinput
> +__xservername__ input driver it is recommended to use the
> +.B LeftHanded
> +option instead. Adjusting the

Adjusting the what?

> +.PP
> +The
> +.B libinput
> +__xservername__ input driver does not use the button mapping after setup.
> +Use XSetPointerMapping(__libmansuffix__) to modify the button mapping at
> +runtime.
> +
>  .SH AUTHORS
>  Peter Hutterer
>  .SH "SEE ALSO"
[...]

Cheers,
Uli

-- 
"For saving the Earth.. and eating cheesecake!"
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-nested 5/5] Introduce a new XCB client backend, and make it the default one.

[Uli Schlachter]

Hi,

Am 31.10.2014 um 14:12 schrieb Laércio de Sousa:
> +case XCB_CONN_CLOSED_REQ_LEN_EXCEED:
> +xf86DrvMsg(scrnIndex,
> +   X_ERROR,
> +   "Connection to host X server closed: too many 
> requests.\n");
> +return TRUE;


This isn't a "too many requests" error, this is "something tried to send a
single request larger than the supported maximum request length" (Think:
"Something tried to send a request of size 1 GiB"). Or, to quote xcb.h:

 /** Connection closed, exceeding request length that server accepts. */
 #define XCB_CONN_CLOSED_REQ_LEN_EXCEED 4

[...]
> +static void
> +_NestedClientSetWindowTitle(NestedClientPrivatePtr pPriv,
> +const char *extra_text)
> +{
[...]
> +xcb_flush(pPriv->conn);
> +}
[...]
> +static Bool
> +_NestedClientHostXInit(NestedClientPrivatePtr pPriv)
> +{
[...]
> +xcb_flush(pPriv->conn);
> +}
[...]

What's your strategy for placing calls to xcb_flush()? I'd like to see each
xcb_flush() having a comment explaining why it is needed.

E.g. _NestedClientSetWindowTitle() calls xcb_flush(), but its only caller,
_NestedClientCreateWindow() does not flush. I can't see why a flush would be
required between setting a window title and mapping the window.

My preferred solution would be to remove all these calls to xcb_flush().
Instead, "something in the event loop" should flush. For example, you could
place a call to xcb_flush() at the end of NestedClientCheckEvents() and remove
all other calls (no idea if this actually works, I don't know the code well 
enough).

> +void
> +NestedClientCheckEvents(NestedClientPrivatePtr pPriv)
> +{
> +xcb_generic_event_t *ev;
> +
> +while (TRUE)
> +{
> +ev = xcb_poll_for_event(pPriv->conn);
> +
> +if (!ev)
> +{
> +if (xcb_connection_has_error(pPriv->conn))
> +exit(1);
[...]

Really? Doesn't this mean "when something goes wrong, we will silently exit
without any error message"?

Cheers,
Uli
-- 
"Why make things difficult, when it is possible to make them cryptic
and totally illogical, with just a little bit more effort?" -- A. P. J.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: What's the right way to call xcb_connect_to_display_with_auth_info() given a Xauthority file?

[Uli Schlachter]

Hi,

Am 17.10.2014 um 14:55 schrieb Laércio de Sousa:
[...]
> I would like to establish this connection without having to set environment
> variable XAUTHORITY. I know there's a funcion in XCB API called
> xcb_connect_to_display_with_auth_info() which receives a
> xcb_auth_info_t struct,
> but I have absolutely no idea of how to build this struct given a
> Xauthority file path.
> 
> How could I do it?
> Thanks in advance!
[...]

I never used this myself, but you could just look into xcb's source code to
figure out how xcb_connect() gets its auth info.

xcb_connect() just calls xcb_connect_with_auth_info() with NULL for the auth
information. This function then uses _xcb_get_auth_info() to get the
xcb_auth_info_t structure:

http://cgit.freedesktop.org/xcb/libxcb/tree/src/xcb_util.c?id=382d306d6c44a9ece5551c210a932773b5cb94a5#n521

The function _xcb_get_auth_info() does some dances to get a Xauth*, then copies
some information from it into the xcb_auth_info_t and calls compute_auth():

http://cgit.freedesktop.org/xcb/libxcb/tree/src/xcb_auth.c?id=382d306d6c44a9ece5551c210a932773b5cb94a5#n344

This function mostly shuffles some bytes around (does anyone use XDM AUTH?, I
guess the AUTH_MC1 case should suffice for 90% of people):

http://cgit.freedesktop.org/xcb/libxcb/tree/src/xcb_auth.c?id=382d306d6c44a9ece5551c210a932773b5cb94a5#n187

Since you just have a Xauthority file path, you could use XauReadAuth() to read
the FILE*. That gives you an Xauth* structure which you can hopefully use in the
same way that libxcb uses it.

Cheers,
Uli
-- 
"Are you preparing for another war, Plutarch?" I ask.
"Oh, not now. Now we're in that sweet period where everyone agrees that our
recent horrors should never be repeated," he says. "But collective thinking is
usually short-lived. We're fickle, stupid beings with poor memories and a great
gift for self-destruction.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] ephyr: Properly implement hardware cursors

[Uli Schlachter]

Hi,

On 21.08.2014 17:07, Keith Packard wrote:
> Adam Jackson  writes:
[...]
>> +xcb_render_pictformat_t
>> +hostx_get_argb_format(void)
> 
> Sigh. Would be nice if there were utility functions similar to those in
> libXrender available for xcb. But, whatever.
[...]

You mean something like xcb-render-util and its
xcb_render_util_find_standard_format() function?

Yeah, would be nice if that existed. :-P

Cheers,
Uli
-- 
A learning experience is one of those things that say,
'You know that thing you just did? Don't do that.'
 -- Douglas Adams
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 2/2] glamor: Use float matrix for render transformations

[Uli Schlachter]

Hi,

On 19.08.2014 00:16, Keith Packard wrote:
> walter harms  writes:
>> So it would be better to use the 16.16 format for transfer and use double
>> for internal calculations ?
> 
> No, the 16.16 format is fundamentally inappropriate in the
> representation of transformation matrices.

Then how about a 16.32 or 16.48 fixed point format? Such a format wouldn't lose
precision with higher numbers. Perhaps a 32.32 fixed point format might make
some (marginal) sense, too.

Why would a floating point format be better than extending the fixed point
format on the wire?

Uli
-- 
"In the beginning the Universe was created. This has made a lot of
 people very angry and has been widely regarded as a bad move."
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Fix XNextRequest() after direct usage of XCB

[Uli Schlachter]

On 10.05.2014 00:21, otay...@redhat.com wrote:
> From: "Owen W. Taylor" 
> 
> When XCB owns the X socket, dpy->request is not updated, so
> NextRequest() and XNextRequest() return the wrong value. There's
> nothing we can do to fix NextRequest() while retaining ABI compat,
> but change XNextRequest() to grab the socket back from XCB,
> updating dpy->request.
> 
> Signed-off-by: Owen W. Taylor 

Reviewed-by: Uli Schlachter 

(And this also makes this function thread safe on 32bit archs were reading a
64bit value isn't atomic)

> ---
>  src/Macros.c  | 14 +-
>  src/Xxcbint.h |  2 ++
>  src/xcb_io.c  | 11 +++
>  3 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/src/Macros.c b/src/Macros.c
> index cfc083a..394a764 100644
> --- a/src/Macros.c
> +++ b/src/Macros.c
> @@ -30,6 +30,7 @@ in this Software without prior written authorization from 
> The Open Group.
>  #include "Xlibint.h"
>  #define XUTIL_DEFINE_FUNCTIONS
>  #include "Xutil.h"
> +#include "Xxcbint.h"
>  
>  /*
>   * This file makes full definitions of routines for each macro.
> @@ -135,9 +136,20 @@ int XBitmapPad(Display *dpy) { return (BitmapPad(dpy)); }
>  
>  int XImageByteOrder(Display *dpy) { return (ImageByteOrder(dpy)); }
>  
> +/* XNextRequest() differs from the rest of the functions here because it is
> + * no longer a macro wrapper - when libX11 is being used mixed together
> + * with direct use of xcb, the next request field of the Display structure 
> will
> + * not be updated. We can't fix the NextRequest() macro in any easy way,
> + * but we can at least make XNextRequest() do the right thing.
> + */
>  unsigned long XNextRequest(Display *dpy)
>  {
> -return (NextRequest(dpy));
> +unsigned long next_request;
> +LockDisplay(dpy);
> +next_request = _XNextRequest(dpy);
> +UnlockDisplay(dpy);
> +
> +return next_request;
>  }
>  
>  unsigned long XLastKnownRequestProcessed(Display *dpy)
> diff --git a/src/Xxcbint.h b/src/Xxcbint.h
> index a8c9a67..bf41c23 100644
> --- a/src/Xxcbint.h
> +++ b/src/Xxcbint.h
> @@ -46,4 +46,6 @@ typedef struct _X11XCBPrivate {
>  int _XConnectXCB(Display *dpy, _Xconst char *display, int *screenp);
>  void _XFreeX11XCBStructure(Display *dpy);
>  
> +unsigned long _XNextRequest(Display *dpy);
> +
>  #endif /* XXCBINT_H */
> diff --git a/src/xcb_io.c b/src/xcb_io.c
> index 727c6c7..5987329 100644
> --- a/src/xcb_io.c
> +++ b/src/xcb_io.c
> @@ -774,3 +774,14 @@ void _XEatDataWords(Display *dpy, unsigned long n)
>   dpy->xcb->reply_consumed = dpy->xcb->reply_length;
>   _XFreeReplyData(dpy, False);
>  }
> +
> +unsigned long
> +_XNextRequest(Display *dpy)
> +{
> +/* This will update dpy->request. The assumption is that the next thing
> + * that the application will do is make a request so there's little
> + * overhead.
> + */
> +require_socket(dpy);
> +return NextRequest(dpy);
> +}
> 


-- 
"Are you preparing for another war, Plutarch?" I ask.
"Oh, not now. Now we're in that sweet period where everyone agrees that our
recent horrors should never be repeated," he says. "But collective thinking is
usually short-lived. We're fickle, stupid beings with poor memories and a great
gift for self-destruction.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Fix XNextRequest() after direct usage of XCB

[Uli Schlachter]

On 09.05.2014 22:33, otay...@redhat.com wrote:
> From: "Owen W. Taylor" 
> 
> When XCB owns the X socket, dpy->request is not updated, so
> NextRequest() and XNextRequest() return the wrong value. There's
> nothing we can do to fix NextRequest() while retaining ABI compat,
> but change XNextRequest() to grab the socket back from XCB,
> updating dpy->request.

Nice idea! We already had/have a problem due to this in cairo. However, one
comment below.

> ---
>  src/Macros.c  | 12 
>  src/Xxcbint.h |  2 ++
>  src/xcb_io.c  | 11 +++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/src/Macros.c b/src/Macros.c
> index cfc083a..b57f577 100644
> --- a/src/Macros.c
> +++ b/src/Macros.c
> @@ -30,6 +30,7 @@ in this Software without prior written authorization from 
> The Open Group.
>  #include "Xlibint.h"
>  #define XUTIL_DEFINE_FUNCTIONS
>  #include "Xutil.h"
> +#include "Xxcbint.h"
>  
>  /*
>   * This file makes full definitions of routines for each macro.
> @@ -135,8 +136,19 @@ int XBitmapPad(Display *dpy) { return (BitmapPad(dpy)); }
>  
>  int XImageByteOrder(Display *dpy) { return (ImageByteOrder(dpy)); }
>  
> +/* XNextRequest() differs from the rest of the functions here because it is
> + * no longer a Macro wrapper - when libX11 is being used mixed together
> + * with direct use of xcb, the next request field of the Display structure 
> will
> + * not be updated; we can't fix the NextRequest() macro in any easy way,
> + * but we can at least make XNextRequest() do the right thing.
> + */
>  unsigned long XNextRequest(Display *dpy)
>  {
> +unsigned long next_request;
> +LockDisplay(dpy);
> +next_request = _XNextRequest(dpy);
> +UnlockDisplay(dpy);
> +
>  return (NextRequest(dpy));

Unused variable "next_request". Did you really mean to write the function like
this instead of also changing the return statement?

>  }
>  
> diff --git a/src/Xxcbint.h b/src/Xxcbint.h
> index a8c9a67..bf41c23 100644
> --- a/src/Xxcbint.h
> +++ b/src/Xxcbint.h
> @@ -46,4 +46,6 @@ typedef struct _X11XCBPrivate {
>  int _XConnectXCB(Display *dpy, _Xconst char *display, int *screenp);
>  void _XFreeX11XCBStructure(Display *dpy);
>  
> +unsigned long _XNextRequest(Display *dpy);
> +
>  #endif /* XXCBINT_H */
> diff --git a/src/xcb_io.c b/src/xcb_io.c
> index 727c6c7..5987329 100644
> --- a/src/xcb_io.c
> +++ b/src/xcb_io.c
> @@ -774,3 +774,14 @@ void _XEatDataWords(Display *dpy, unsigned long n)
>   dpy->xcb->reply_consumed = dpy->xcb->reply_length;
>   _XFreeReplyData(dpy, False);
>  }
> +
> +unsigned long
> +_XNextRequest(Display *dpy)
> +{
> +/* This will update dpy->request. The assumption is that the next thing
> + * that the application will do is make a request so there's little
> + * overhead.
> + */
> +require_socket(dpy);
> +return NextRequest(dpy);
> +}
> 


-- 
"Are you preparing for another war, Plutarch?" I ask.
"Oh, not now. Now we're in that sweet period where everyone agrees that our
recent horrors should never be repeated," he says. "But collective thinking is
usually short-lived. We're fickle, stupid beings with poor memories and a great
gift for self-destruction.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 0/3] Fix intra-xcb .pc file dependencies automatically

[Uli Schlachter]

On 12.02.2014 23:15, Keith Packard wrote:
>> However, I don't like the result. composite.xml ""s only xproto and
>> xfixes. The dependency on render and shape occur because xfixes depends on
>> these. In other words, your tool is including transitive dependencies.
> 
> I've patched that by fixing xcb to only #include files for direct
> dependencies. The transitive #include set will still include all of
> the dependencies, of course. That's the first patch in this
> series. This should also lead to a (trivial) compile time improvement
> as we we'll process the nested header files fewer times.
> 
> This (obviously) requires a separate patch to the xcb proto files,
> which I've already sent under separate cover. That patch simply adds
> the direct_import field needed by this patch and must be applied
> first. "Also track directly imported modules in a separate list"
> 
>  [PATCH 1/3] Only #include directly referenced module header files
> 
> 
>>  (Also, your tool will (silently) fail with out-of-tree builds, because 
>> either
>>  the .pc.in or the src/*.h files can't be found. I guess automake runs tests 
>> in
>>  the build dir, but I am not sure. And I don't care a lot about this issue 
>> since
>>  the script is already complicated enough).
> 
> I've fixed that in this updated patch:
> 
>  [PATCH 2/3] Validate .pc file Requires lines
> 
> Finally, you'll see that the .pc files require only a few changes now
> that they don't include the transitive closure of dependencies
> 
>  [PATCH 3/3] Update .pc file Requires lines to express full

Merged as cb686b5..e2813e1  master -> master.

The problem with "Requires" vs "Requires.private" is independent of this change
and can be handled on its own. Which likely means that it will just be 
ignored...

Cheers,
Uli
-- 
Who needs a ~/.signature anyway?
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [GSoC] Project idea: C++ bindings for XCB

[Uli Schlachter]

Hi,

On 01.03.2014 10:06, Alexander Mezin wrote:
[...]
> I have my own idea for project, not listed on ideas page.
> I want to write C++ binding/wrapper for XCB.
> When I worked on another GSoC project previous year (touchpad
> configuration utility for KDE [1]), I had to work with libxcb from C++
> code.
> I've noticed that libxcb's interface nicely maps to RAII idiom, but
> working with libxcb C API requires a lot of boilerplate code.
> I've found some C++ wrappers for libxcb, but it seems that they aren't
> actively developed.
> 
> I think that the best way is to use xcb protocol descriptions to
> generate C++ wrappers, which will internally use libxcb.
> I don't like templates as much as Boost developers do, so I try to
> avoid them when possible.
> Maybe it would be good to write C++ wrappers for utility libraries also.
[...]

I like this idea. The "nicely maps to RAII idiom" made me remember something
nice that I saw in kwin's source code. They have a "Wrapper" template which does
exactly that:

https://projects.kde.org/projects/kde/kde-workspace/repository/revisions/7bacb97404d9a55194da2490d7266b3264d6638c/entry/kwin/xcbutils.h#L50

I know that your proposal goes way beyond this, but I just wanted to mention the
API that they came up with. Of course, yours will be much nicer. :-)

Good luck and have fun,
Uli
-- 
"Why make things difficult, when it is possible to make them cryptic
and totally illogical, with just a little bit more effort?" -- A. P. J.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Xcb] [PATCH 2/2] Update .pc file Requires lines to express full dependencies

[Uli Schlachter]

On 12.02.2014 21:14, Keith Packard wrote:
> Some xcb libraries depend on others; make these dependencies explicit
> in the .pc files that are installed.
> 
> This change was generated automatically by running 'check-pc-requires -fix'
> 
> Signed-off-by: Keith Packard 
> ---
>  xcb-composite.pc.in | 2 +-
>  xcb-damage.pc.in| 2 +-
>  xcb-present.pc.in   | 2 +-
>  xcb-randr.pc.in | 2 +-
>  xcb-xinput.pc.in| 2 +-
>  xcb-xvmc.pc.in  | 2 +-
>  6 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/xcb-composite.pc.in b/xcb-composite.pc.in
> index 02d49b0..d266c7c 100644
> --- a/xcb-composite.pc.in
> +++ b/xcb-composite.pc.in
> @@ -6,6 +6,6 @@ includedir=@includedir@
>  Name: XCB Composite
>  Description: XCB Composite Extension
>  Version: @PACKAGE_VERSION@
> -Requires: xcb xcb-xfixes
> +Requires: xcb xcb-xfixes xcb-render xcb-shape
>  Libs: -L${libdir} -lxcb-composite
>  Cflags: -I${includedir}
[...]

First: Thanks a lot for looking into this!

However, I don't like the result. composite.xml ""s only xproto and
xfixes. The dependency on render and shape occur because xfixes depends on
these. In other words, your tool is including transitive dependencies.

However, these aren't dependencies of xcb-composite itself and pkg-config is
smart enough to resolve transitive dependencies by itself:

$ pkg-config --libs xcb-composite
-L/usr/local/lib -lxcb-composite -lxcb-xfixes -lxcb-render -lxcb-shape -lxcb

Since I don't see any reason for including transitive deps, I would vote for not
including them.

Hopefully this simplifies the .sh script since right now I have no idea what
exactly the different parts are doing. Also, for that script I'd like propose a
little help text at the end. Something like "Run `$0 -fix' to automatically fix
these issues" if any complaints were generated.

(Also, your tool will (silently) fail with out-of-tree builds, because either
the .pc.in or the src/*.h files can't be found. I guess automake runs tests in
the build dir, but I am not sure. And I don't care a lot about this issue since
the script is already complicated enough).

(Oh and stylistic nitpick: I prefer 'if "x$foo" = x ;' over the case-s, but
since this is personal taste, this can be ignored, too.)

Thanks!
Uli
-- 
No matter how much cats fight, there always seem to be plenty of kittens.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11 2/2] xcb_io: Add comment explaining a mixed type double assignment

[Uli Schlachter]

Hi,

On 16.11.2013 22:37, Jonas Petersen wrote:
> The assignment might be confusing at first. So I added a note.
> 
> Signed-off-by: Jonas Petersen 
> ---
>  src/xcb_io.c |4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/xcb_io.c b/src/xcb_io.c
> index f2978d0..acb1e3b 100644
> --- a/src/xcb_io.c
> +++ b/src/xcb_io.c
> @@ -83,6 +83,10 @@ static void require_socket(Display *dpy)
>"did not own the socket",
>xcb_xlib_seq_number_wrapped);
>   }
> + /* The following line will truncate the 64-bit 'sent'
> +  * to 32-bit when assigning it to 'dpy->request'. The
> +  * truncated value will then be assigned to the 64-bit
> +  * 'dpy->xcb->last_flushed' (which is intended). */
>   dpy->xcb->last_flushed = dpy->request = sent;
>   dpy->bufmax = dpy->xcb->real_bufmax;
>   }

The involved types are:

   uint64_t sent;
   uint64_t last_flushed;
   unsigned long request;

So request isn't really a 32-bit type. In fact, on my system, all of these three
are the same type. So the comment doesn't really fit.

Now let's assume that unsigned long is a 32-bit integer. *Why* is it intended to
truncate last_flushed? This means that Xlib uses a 64 bit integer for to
tracking the sequence number, but only actually tracks it with 32 bits. This
seems wasteful. Why are you sure that this is intended?

Disclaimer: I don't know too much about Xlib internals and were not involved in
writing xcb_io.c

Cheers,
Uli
-- 
"Every once in a while, declare peace. It confuses the hell out of your enemies"
 - 79th Rule of Acquisition
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping

[Uli Schlachter]

On 16.11.2013 22:37, Jonas Petersen wrote:
> By design the Xlib 32-bit internal request sequence numbers may wrap. There
> is two locations within xcb_io.c that are not wrap-safe. The value of
> last_flushed relies on the request to be sequential all the time. This is
> not given when the sequence has just wrapped. Applications may then crash
> with a "Fatal IO error 11 (Resource temporarily unavailable)".
> 
> This patch fixes this by unwrapping the sequence number when needed to retain
> the sequence relative to last_flushed.
> 
> Signed-off-by: Jonas Petersen 
> ---
>  src/xcb_io.c |   10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/xcb_io.c b/src/xcb_io.c
> index 727c6c7..f2978d0 100644
> --- a/src/xcb_io.c
> +++ b/src/xcb_io.c
> @@ -455,7 +455,7 @@ void _XSend(Display *dpy, const char *data, long size)
>   static const xReq dummy_request;
>   static char const pad[3];
>   struct iovec vec[3];
> - uint64_t requests;
> + uint64_t requests, unwrapped_request;
>   _XExtension *ext;
>   xcb_connection_t *c = dpy->xcb->connection;
>   if(dpy->flags & XlibDisplayIOError)
> @@ -464,6 +464,10 @@ void _XSend(Display *dpy, const char *data, long size)
>   if(dpy->bufptr == dpy->buffer && !size)
>   return;
>  
> + /* Set bit 8 of 'request' when a 32-bit wrap has just happened
> +  * so the sequence stays correct relatively to 'last_flushed'. */

"1 << 32" does not set bit 8 and this doesn't set anything in request, really.

> + unwrapped_request = ((uint64_t)(dpy->request < dpy->xcb->last_flushed) 
> << 32) + dpy->request;

Also, I don't think that this code is intuitive this way.

I would propose this instead:

  unwrapped_request = dpy->request;
  /* If there was a sequence number wrap since our last flush, make sure we
   * use the correct 64 bit sequence number */
  if (sizeof(uint64_t) > sizeof(unsigned long)
 && dpy->request < dpy->xcb_last_flushed)
 unwrapped_request += UINT64_C(1) << 32;

(I am not sure/convinced if the sizeof comparision is necessary, but I saw
something like this in require_socket() and then thought that this might be
necessary on systems where unsigned long already is a 64 bit type.)

>   /* iff we asked XCB to set aside errors, we must pick those up
>* eventually. iff there are async handlers, we may have just
>* issued requests that will generate replies. in either case,
> @@ -471,10 +475,10 @@ void _XSend(Display *dpy, const char *data, long size)
>   if(dpy->xcb->event_owner != XlibOwnsEventQueue || dpy->async_handlers)
>   {
>   uint64_t sequence;
> - for(sequence = dpy->xcb->last_flushed + 1; sequence <= 
> dpy->request; ++sequence)
> + for(sequence = dpy->xcb->last_flushed + 1; sequence <= 
> unwrapped_request; ++sequence)
>   append_pending_request(dpy, sequence);
>   }
> - requests = dpy->request - dpy->xcb->last_flushed;
> + requests = unwrapped_request - dpy->xcb->last_flushed;
>   dpy->xcb->last_flushed = dpy->request;
>  
>   vec[0].iov_base = dpy->buffer;
> 


-- 
"Every once in a while, declare peace. It confuses the hell out of your enemies"
 - 79th Rule of Acquisition
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Xcb] [PATCH 0/8] Updated version of DRI3/Present additions to libxcb

[Uli Schlachter]

On 07.11.2013 14:38, Keith Packard wrote:
> Uli Schlachter  writes:
> 
>> All the patches except for patch 4 (with my comments included and tabs
>> removed):
> 
> I think I'm all set; I've rebased to 1.9 (adding some master patches
> which make it compile with new xproto).

Thanks. If no one else complains, I don't mind a new release (but, as I said
before, I don't think it's my job to decide this, so everyone who considers
themselves a XCB dev may now speak up).

> What else would you like to see in the send_fd patch?

I guess I am more or less convinced, that sending fds works as expected and I
trust you that partial reads are handled correctly for fds coming from the
server. (You can count this as a Reviewed-By ;-) )

So no more complaints from me and sorry for being annoying. :-)

Happy releasing,
Uli
-- 
"For saving the Earth.. and eating cheesecake!"
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Xcb] [PATCH 4/8] Add xcb_send_fd API

[Uli Schlachter]

On 07.11.2013 13:24, Keith Packard wrote:

Uli Schlachter  writes:


Having thought about this some more in the shower, I would suggest to turn
xcb_send_fd() into a private _send_fd() and instead add
xcb_send_request_with_fds() (or something like that).


This isn't necessary; as I explained, the X server just holds onto FDs
that the client sends until a request consumes them. Yes, it has to deal
with clients sending unexpected FDs, there's nothing special about this.


Ok, but then xcb needs to do the same (see below).

[...]

- Does the new sendmsg() call work correctly on non-unix sockets when no FDs are
transmitted?


You'll note that sendmsg is *not* called when we have no file
descriptors to send.


Ah, sorry, I did indeed miss that part.


- Should we have some API to test if sending FDs works at all or if xcb just
ignores the FDs?


I'm not entirely sure how you'd do that without actually trying to send
an FD, given that XCB can be handed a pre-connected file descriptor.


- What happens with partial reads? Let's say we send 4KiB of requests to the
server in one sendmsg() call and the last request has FDs attached. For whatever
reason, the server only reads 2KiB of data from the socket. Will the FDs be
handled correctly in this case? What if the FDs are for the first request
instead of the last one?


The FDs are effectively attached to the first byte of the write; read
that bytes and you get all of the FDs sent there. So, if you perform
a 4kB write with 10 file descriptors, and read only 2kB, then you'll get
all 10 file descriptors as you've read the relevant byte.


Ok, seems like the server handles this correctly then.

However, now I wonder about patch 5/8 "Add support for receiving fds" which does 
not do this.


If I understood the code correctly, _xcb_in_read() will close the connection 
with an error if a partial read happens and thus we get FDs before the 
corresponding event/reply.


In other words, the server handles partial reads correctly, xcb doesn't


Oh and I just noticed:
If HAVE_SENDMSG is not defined, shouldn't xcb_send_fd() close() the FD
that it gets?


The X server won't advertise any requests which receive or send file
descriptors on a system which doesn't support SENDMSG, so I'm not sure
how interesting the precise semantics of this error case are. And, I
assume the BSD people will shortly figure out how to make this work on
their operating systems, which means that pretty much anything with
local applications will have working xcb FD passing.


I've seen lots of code which just uses an extension without checking for its 
availability. I am sure people will be able to get this wrong, too. At least I 
hope that this would then cause an error from the server and thus gets noticed...


Anyway, I guess I ran out of doubts for this patch (and now have new doubts 
about patch 5/8), so:


Reviewed-By: Uli Schlachter 

Uli
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Xcb] [PATCH 6/8] Add event queue splitting

[Uli Schlachter]

Hi again,

just to make sure that I understood this API correctly, let me try to write some
documentation:

On 07.11.2013 11:14, Uli Schlachter wrote:
> On 07.11.2013 04:45, Keith Packard wrote:
[...]
>> +/**
>> + * @brief Returns the next event from a special queue
>> + */
>> +xcb_generic_event_t *xcb_poll_for_special_event(xcb_connection_t *c,
>> +xcb_special_event_t *se);
>> + 
>> +/**
>> + * @brief Returns the next event from a special queue, blocking until one 
>> arrives
>> + */
>> +xcb_generic_event_t *xcb_wait_for_special_event(xcb_connection_t *c,
>> +xcb_special_event_t *se);
>> + 
>> +/**
>> + * @brief Listen for a special event
>> + */

Listen for a special event. Any events matching these conditions will be
filtered out of the normal event stream and be available only through
xcb_poll_for_special_event() and xcb_wait_for_special_event().
@param c The xcb connection.
@param extension The major number of the extension that generates this event.
@param eid The event type of the GE event that should be filtered
@param stamp A variable that will be incremented every time that a new event is
received. This update will only be protected internally to libxcb. This means
that you should never modify this value directly and may only read it
atomically. This argument may be NULL in which case no increments are performed.
@return A handle to the filter that this function installed or NULL. This must
be destroyed via xcb_unregister_for_special_event().

Having written the above, I wonder if the first argument should be a struct
xcb_extension_t instead of a major number. I think that this would be the first
function to use a major number like this and it is trivial for the
implementation to turn the xcb_extension_t into a major number.

>> +xcb_special_event_t *xcb_register_for_special_xge(xcb_connection_t *c,
>> +  uint8_t extension,
>> +  uint32_t eid,
>> +  uint32_t *stamp);
>> +
>> +/**
>> + * @brief Stop listening for a special event
>> + */
>> +void xcb_unregister_for_special_event(xcb_connection_t *c,
>> +  xcb_special_event_t *se);
[...]

Oh and: Could you please add if(!se) return; to this function? This could
simplify the error handling of library users.

With this change applied:

Stop listening for a special event. This undoes the effect of
xcb_register_for_special_xge().
@param c The xcb connection.
@param se A value returned from xcb_register_for_special_xge(). NULL is allowed

-- 
A normal person is just someone you don't know well enough yet.
 - Nettie Wiebe
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Xcb] [PATCH 4/8] Add xcb_send_fd API

[Uli Schlachter]

On 07.11.2013 10:03, Uli Schlachter wrote:
> On 07.11.2013 04:45, Keith Packard wrote:
[...]
>> +void
>> +xcb_send_fd(xcb_connection_t *c, int fd)
>> +{
>> +#if HAVE_SENDMSG
> 
>   if (c->has_error)
> return;
> 
> This is needed before the pthread_mutex_lock(), because if
> xcb_connect_to_display() fails, it returns a pointer to an integer. The
> has_error 'field' is the only thing that might be accessed for such a 
> connection
> object.
> 
>> +pthread_mutex_lock(&c->iolock);
>> +while (c->out.out_fd.nfd == XCB_MAX_PASS_FD) {
>> +_xcb_out_flush_to(c, c->out.request);
>> +if (c->has_error)
>> +break;
>> +}
>> +if (!c->has_error)
>> +c->out.out_fd.fd[c->out.out_fd.nfd++] = fd;
>> +pthread_mutex_unlock(&c->iolock);
>> +#endif
>> +}
> 
> So, xcb_send_fd() hands FDs to libxcb which will all be included in the next
> flush. The idea is that the user of the library calls (through the 
> automatically
> generated protocol functions) xcb_send_fd() and then xcb_send_request(). 
> Correct?
> 
> However, what happens if the connection is flushed between the xcb_send_fd() 
> and
> the xcb_send_request() calls, because another thread "interferes"? This can 
> also
> happen with a single thread: xcb_send_request() could have to insert sync
> requests (GetInputFocus) because of sequence number 0 or sequence number 
> wraps.
> This could cause the output buffer to go full before the "real" request is 
> inserted.
> 
> I guess that the server expects the FDs to be sent with the request that they
> are meant to (otherwise I could feed the server lots of FDs which it would 
> have
> to keep around forever). So this function doesn't really work, does it?
[...]

Having thought about this some more in the shower, I would suggest to turn
xcb_send_fd() into a private _send_fd() and instead add
xcb_send_request_with_fds() (or something like that).

This way, send_request() can call _send_fd() internally after it has sent all
the needed syncs and with the mutex held. In other words: In a way that makes
sure that nothing is flushed prematurely.

However, I also noticed that I don't know much about "unix fd magic", so could
anyone please answer some questions:

- What happens if we try to send FDs over non-unix sockets?
- Does the new sendmsg() call work correctly on non-unix sockets when no FDs are
transmitted?
- Should we have some API to test if sending FDs works at all or if xcb just
ignores the FDs?
- What happens with partial reads? Let's say we send 4KiB of requests to the
server in one sendmsg() call and the last request has FDs attached. For whatever
reason, the server only reads 2KiB of data from the socket. Will the FDs be
handled correctly in this case? What if the FDs are for the first request
instead of the last one?
(Of course, this last question also applies to FDs that the server sends to 
libxcb)

Oh and I just noticed:
If HAVE_SENDMSG is not defined, shouldn't xcb_send_fd() close() the FD that it 
gets?

-- 
A normal person is just someone you don't know well enough yet.
 - Nettie Wiebe
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Xcb] [PATCH 0/8] Updated version of DRI3/Present additions to libxcb

[Uli Schlachter]

On 07.11.2013 04:45, Keith Packard wrote:
> I've generated a new sequence of patches in response to the very
> helpful review comments received so far. Thanks to all involved!
> 
> Some simple cleanups that I have found useful. Patch 3/8 has been
> split out from the event queue patch and matches a change made to the
> XCB proto that adds an XML description of the X generic event
> structure. Seems like a sensible plan to me, given that the protocol
> generator already knows about the XGE fields
> 
>  [PATCH 1/8] Make protocol C files depend on protocol XML files
>  [PATCH 2/8] -pendantic is too pendantic
>  [PATCH 3/8] Remove xcb_ge_event_t from xcb.h

Oh, this means we also need a new xcb-proto release. That should have been clear
to anyone thinking about all of this for 0.5 seconds, but I totally missed that.
This means we need to bump our minimum required version of xcb-proto in libxcb.

Is there anything in xcb-proto's git master which isn't ready releasable?

> Deal with fd passing. This is now enabled only on Linux as the recvmsg
> code does not support non-Linux systems using the CMSG APIs. A
> configure.ac variable is also made available to disable extensions
> which require FD passing (like DRI3).
> 
>  [PATCH 4/8] Add xcb_send_fd API
>  [PATCH 5/8] Add support for receiving fds in replies
> 
> Here's the event queue splitting API. The polling function is now
> called xcb_poll_for_special_event, and the XGE filtering creator is
> now called xcb_register_for_special_xge.
> 
>  [PATCH 6/8] Add event queue splitting
> 
> DRI3 is now enabled only when sendmsg is usable for FD passing
> (Linux-only, until the sendmsg/recvmsg bits are fixed for non-Linux systems).
> 
>  [PATCH 7/8] Add DRI3 library
>  [PATCH 8/8] Add Present extension

All the patches except for patch 4 (with my comments included and tabs removed):

Reviewed-By: Uli Schlachter 

Uli
-- 
"In the beginning the Universe was created. This has made a lot of
 people very angry and has been widely regarded as a bad move."
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Xcb] [PATCH 6/8] Add event queue splitting

[Uli Schlachter]

On 07.11.2013 04:45, Keith Packard wrote:
> This allows apps to peel off certain XGE events into separate queues
> for custom handling. Designed to support the Present extension
> 
> Signed-off-by: Keith Packard 
> ---
>  src/xcb.h|  28 ++
>  src/xcb_in.c | 167 
> +--
>  src/xcbint.h |   1 +
>  3 files changed, 193 insertions(+), 3 deletions(-)
> 
> diff --git a/src/xcb.h b/src/xcb.h
> index c251330..cde820d 100644
> --- a/src/xcb.h
> +++ b/src/xcb.h
> @@ -287,6 +287,34 @@ xcb_generic_event_t *xcb_poll_for_event(xcb_connection_t 
> *c);
>   */
>  xcb_generic_event_t *xcb_poll_for_queued_event(xcb_connection_t *c);
>  
> +typedef struct xcb_special_event xcb_special_event_t;

For bikeshedding, I would call this xcb_special_event_filter_t, because this is
not actually an event. Having said that, feel free to ignore me.

> +/**
> + * @brief Returns the next event from a special queue
> + */
> +xcb_generic_event_t *xcb_poll_for_special_event(xcb_connection_t *c,
> +xcb_special_event_t *se);
> + 
> +/**
> + * @brief Returns the next event from a special queue, blocking until one 
> arrives
> + */
> +xcb_generic_event_t *xcb_wait_for_special_event(xcb_connection_t *c,
> +xcb_special_event_t *se);
> + 
> +/**
> + * @brief Listen for a special event
> + */
> +xcb_special_event_t *xcb_register_for_special_xge(xcb_connection_t *c,
> +  uint8_t extension,
> +  uint32_t eid,
> +  uint32_t *stamp);
> +
> +/**
> + * @brief Stop listening for a special event
> + */
> +void xcb_unregister_for_special_event(xcb_connection_t *c,
> +  xcb_special_event_t *se);
> +
>  /**
>   * @brief Return the error for a request, or NULL if none can ever arrive.
>   * @param c: The connection to the X server.
> diff --git a/src/xcb_in.c b/src/xcb_in.c
> index 0a78ae6..9239c93 100644
> --- a/src/xcb_in.c
> +++ b/src/xcb_in.c
> @@ -61,6 +61,23 @@ struct event_list {
>  struct event_list *next;
>  };
>  
> +struct xcb_special_event {
> +
> +struct xcb_special_event *next;
> +
> +/* Match XGE events for the specific extension and event ID (the
> + * first 32 bit word after evtype)
> + */
> +uint8_t extension;
> +uint32_teid;
> +uint32_t*stamp;
> +
> +struct event_list   *events;
> +struct event_list   **events_tail;
> +
> +pthread_cond_t special_event_cond;
> +};
> +
>  struct reply_list {
>  void *reply;
>  struct reply_list *next;
> @@ -105,6 +122,46 @@ static int read_fds(xcb_connection_t *c, int *fds, int 
> nfd)
>  }
>  #endif
>  
> +typedef struct xcb_ge_special_event_t {
> +uint8_t  response_type; /**<  */
> +uint8_t  extension; /**<  */
> +uint16_t sequence; /**<  */
> +uint32_t length; /**<  */
> +uint16_t evtype; /**<  */
> +uint8_t  pad0[2]; /**< */
> +uint32_t eid; /**< */
> +uint8_t  pad1[16]; /**<  */
> +} xcb_ge_special_event_t;
> +
> +static int event_special(xcb_connection_t *c,
> + struct event_list *event)
> +{
> +struct xcb_special_event *special_event;
> +struct xcb_ge_special_event_t *ges = (void *) event->event;
> +
> +/* Special events are always XGE events */
> +if ((ges->response_type & 0x7f) != XCB_XGE_EVENT)
> +return 0;
> +
> +for (special_event = c->in.special_events;
> + special_event;
> + special_event = special_event->next)
> +{
> +if (ges->extension == special_event->extension &&
> +ges->eid == special_event->eid)
> +{
> +*special_event->events_tail = event;
> +special_event->events_tail = &event->next;
> +if (special_event->stamp)
> +++(*special_event->stamp);
> +pthread_cond_signal(&special_event->special_event_cond);
> +return 1;
> +}
> +}
> +
> +return 0;
> +}
> +
>  static int read_packet(xcb_connection_t *c)
>  {
>  xcb_generic_reply_t genrep;
> @@ -269,9 +326,12 @@ static int read_packet(xcb_connection_t *c)
>  }
>  event->event = buf;
>  event->next = 0;
> -*c->in.events_tail = event;
> -c->in.events_tail = &event->next;
> -pthread_cond_signal(&c->in.event_cond);
> +
> +if (!event_special(c, event)) {
> +*c->in.events_tail = event;
> +c->in.events_tail = &event->next;
> +pthread_cond_signal(&c->in.event_cond);
> +}
>  return 1; /* I have something for you... */
>  }
>  
> @@ -614,6 +674,107 @@ xcb_generic_error_t *xcb_request_check(xcb_connection_t 
> *c, xcb_void_cookie_t co
>  return ret;
>  }
>  
> +static xcb_generic_event_t *get_special_event(xcb_connection_t *c,
> + 

Re: [Xcb] [PATCH 5/8] Add support for receiving fds in replies

[Uli Schlachter]

On 07.11.2013 04:45, Keith Packard wrote:
> Requests signal which replies will have fds, and the replies report
> how many fds they expect in byte 1.
> 
> Signed-off-by: Keith Packard 
[...]
> @@ -432,6 +463,11 @@ void *xcb_wait_for_reply(xcb_connection_t *c, unsigned 
> int request, xcb_generic_
>  return ret;
>  }
>  
> +int *xcb_get_reply_fds(xcb_connection_t *c, void *reply, size_t reply_size)
> +{
> +return (int *) (&((char *) reply)[reply_size]);
> +}
> +
>  static void insert_pending_discard(xcb_connection_t *c, pending_reply 
> **prev_next, uint64_t seq)
>  {
>  pending_reply *pend;
> @@ -666,11 +702,68 @@ void _xcb_in_replies_done(xcb_connection_t *c)
>  
>  int _xcb_in_read(xcb_connection_t *c)
>  {
> -int n = recv(c->fd, c->in.queue + c->in.queue_len, sizeof(c->in.queue) - 
> c->in.queue_len, 0);
> -if(n > 0)
> +int n;
> +
> +#if HAVE_SENDMSG
> +struct ioveciov = {
> +.iov_base = c->in.queue + c->in.queue_len,
> +.iov_len = sizeof(c->in.queue) - c->in.queue_len,
> +};
> +struct msghdr msg = {
> +.msg_name = NULL,
> +.msg_namelen = 0,
> +.msg_iov = &iov,
> +.msg_iovlen = 1,
> +.msg_control = &c->in.in_fd,
> +.msg_controllen = sizeof (struct cmsghdr) + sizeof (int) * 
> XCB_MAX_PASS_FD,
> +};
> +n = recvmsg(c->fd, &msg, 0);
>
> +
> +/* Check for truncation errors. Only MSG_CTRUNC is
> + * probably possible here, which would indicate that
> + * the sender tried to transmit more than XCB_MAX_PASS_FD
> + * file descriptors.
> + */
> +if (msg.msg_flags & (MSG_TRUNC|MSG_CTRUNC)) {
> +_xcb_conn_shutdown(c, XCB_CONN_ERROR);
> +return 0;

Could you introduce a new error value for this case, please? It would already be
quite hard to figure out that the connection was closed due to such an error and
without a clear error value, it will be quite impossible.

> +}
> +#else
> +n = recv(c->fd, c->in.queue + c->in.queue_len, sizeof(c->in.queue) - 
> c->in.queue_len, 0);
> +#endif
> +if(n > 0) {
> +#if HAVE_SENDMSG
> +if (msg.msg_controllen > sizeof (struct cmsghdr))
> +{
> +if (c->in.in_fd.cmsghdr.cmsg_level == SOL_SOCKET &&
> +c->in.in_fd.cmsghdr.cmsg_type == SCM_RIGHTS &&
> +!((msg.msg_flags & MSG_TRUNC) ||
> +  (msg.msg_flags & MSG_CTRUNC)))
> +{
> +c->in.in_fd.nfd = (msg.msg_controllen - sizeof (struct 
> cmsghdr)) / sizeof (int);
> +}
> +}
> +#endif
>  c->in.queue_len += n;
> +}
>  while(read_packet(c))
>  /* empty */;
> +#if HAVE_SENDMSG
> +c->in.in_fd.nfd -= c->in.in_fd.ifd;
> +c->in.in_fd.ifd = 0;
> +
> +/* If we have any left-over file descriptors, then
> + * the server sent some that we weren't expecting.
> + * Close them and mark the connection as broken;
> + */
> +if (c->in.in_fd.nfd != 0) {
> +int i;
> +for (i = 0; i < c->in.in_fd.nfd; i++)
> +close(c->in.in_fd.fd[i]);
> +_xcb_conn_shutdown(c, XCB_CONN_ERROR);

I don't like this to be a generic error, too. Could this get the same error
value as the above _xcb_conn_shutdown()? Something like
XCB_CONN_CLOSED_FDPASSING_FAILED (no, I don't really like that name a lot, but I
don't have any better ideas right now).

> +return 0;
> +}
> +#endif
>  #ifndef _WIN32
>  if((n > 0) || (n < 0 && errno == EAGAIN))
>  #else
> diff --git a/src/xcbext.h b/src/xcbext.h
> index 44030c3..1eb1be7 100644
> --- a/src/xcbext.h
> +++ b/src/xcbext.h
> @@ -54,7 +54,8 @@ typedef struct {
>  enum xcb_send_request_flags_t {
>  XCB_REQUEST_CHECKED = 1 << 0,
>  XCB_REQUEST_RAW = 1 << 1,
> -XCB_REQUEST_DISCARD_REPLY = 1 << 2
> +XCB_REQUEST_DISCARD_REPLY = 1 << 2,
> +XCB_REQUEST_REPLY_FDS = 1 << 3
>  };
>  
>  unsigned int xcb_send_request(xcb_connection_t *c, int flags, struct iovec 
> *vector, const xcb_protocol_request_t *request);
> @@ -91,6 +92,7 @@ int xcb_writev(xcb_connection_t *c, struct iovec *vector, 
> int count, uint64_t re
>  
>  void *xcb_wait_for_reply(xcb_connection_t *c, unsigned int request, 
> xcb_generic_error_t **e);
>  int xcb_poll_for_reply(xcb_connection_t *c, unsigned int request, void 
> **reply, xcb_generic_error_t **error);
> +int *xcb_get_reply_fds(xcb_connection_t *c, void *reply, size_t replylen);
>  
>  
>  /* xcb_util.c */
> diff --git a/src/xcbint.h b/src/xcbint.h
> index b2dfed3..bbc5398 100644
> --- a/src/xcbint.h
> +++ b/src/xcbint.h
> @@ -88,6 +88,7 @@ typedef struct _xcb_fd {
>  struct cmsghdr cmsghdr;
>  int fd[XCB_MAX_PASS_FD];
>  int nfd;
> +int ifd;
>  } _xcb_fd;
>  #endif
>  
> @@ -146,6 +147,9 @@ typedef struct _xcb_in {
>  
>  struct pending_reply *pending_replies;
>  struct pending_reply **pending_replies_tail;
> +#if HAVE_SENDMSG
> +_xcb_fd in_fd;
> +#endif
>  } _xcb_in;
>  
>  int _xcb_in_init(_xcb_in *

Re: [Xcb] [PATCH 4/8] Add xcb_send_fd API

[Uli Schlachter]

On 07.11.2013 04:45, Keith Packard wrote:
> This uses sendmsg to transmit file descriptors from the application to
> the X server
> 
> Signed-off-by: Keith Packard 
[...]
> diff --git a/src/xcb_auth.c b/src/xcb_auth.c
> index a5b730c..2f7c93e 100644
> --- a/src/xcb_auth.c
> +++ b/src/xcb_auth.c
> @@ -34,6 +34,12 @@
>  #include 
>  #include 
>  #include 
> +#ifndef _WIN32
> +#include 
> +#include 
> +#include 
> +#include 
> +#endif
>  
>  #ifdef __INTERIX
>  /* _don't_ ask. interix has INADDR_LOOPBACK in here. */
> @@ -47,11 +53,6 @@
>  #include 
>  #endif
>  #include "xcb_windefs.h"
> -#else
> -#include 
> -#include 
> -#include 
> -#include 
>  #endif /* _WIN32 */
>  
>  #include "xcb.h"

The above seems to be an unrelated change. If this is needed due to some
xcb-private header, then that header needs to be fixed. Otherwise this seems
unneeded (for this patch).

> diff --git a/src/xcb_conn.c b/src/xcb_conn.c
> index 7dd25d3..0237d16 100644
> --- a/src/xcb_conn.c
> +++ b/src/xcb_conn.c
> @@ -38,6 +38,11 @@
>  #include 
>  #include 
>  
> +#ifndef _WIN32
> +#include 
> +#include 
> +#endif /* !_WIN32 */
> +
>  #include "xcb.h"
>  #include "xcbint.h"
>  #if USE_POLL
> @@ -48,9 +53,6 @@
>  
>  #ifdef _WIN32
>  #include "xcb_windefs.h"
> -#else
> -#include 
> -#include 
>  #endif /* _WIN32 */

Same here.

>  /* SHUT_RDWR is fairly recent and is not available on all platforms */
> @@ -214,9 +216,34 @@ static int write_vec(xcb_connection_t *c, struct iovec 
> **vector, int *count)
>  if (n > IOV_MAX)
>   n = IOV_MAX;
>  
> -n = writev(c->fd, *vector, n);
> -if(n < 0 && errno == EAGAIN)
> -return 1;
> +#if HAVE_SENDMSG
> +if (c->out.out_fd.nfd) {
> + struct msghdr msg = {
> + .msg_name = NULL,
> + .msg_namelen = 0,
> + .msg_iov = *vector,
> + .msg_iovlen = n,
> + .msg_control = &c->out.out_fd,
> + .msg_controllen = sizeof (struct cmsghdr) + c->out.out_fd.nfd * 
> sizeof (int),
> + };
> +int i;
> + c->out.out_fd.cmsghdr.cmsg_len = msg.msg_controllen;
> + c->out.out_fd.cmsghdr.cmsg_level = SOL_SOCKET;
> + c->out.out_fd.cmsghdr.cmsg_type = SCM_RIGHTS;
> + n = sendmsg(c->fd, &msg, 0);
> +if(n < 0 && errno == EAGAIN)
> +return 1;
> +for (i = 0; i < c->out.out_fd.nfd; i++)
> +close(c->out.out_fd.fd[i]);
> +c->out.out_fd.nfd = 0;
> +} else
> +#endif
> +{
> + n = writev(c->fd, *vector, n);
> +if(n < 0 && errno == EAGAIN)
> +return 1;
> +}
> +
>  #endif /* _WIN32 */
>  
>  if(n <= 0)

This indents with tabs. I know that there is no coding style documented
anywhere, but "most" of libxcb only uses spaces.

> diff --git a/src/xcb_ext.c b/src/xcb_ext.c
> index 831f283..1ff2b05 100644
> --- a/src/xcb_ext.c
> +++ b/src/xcb_ext.c
> @@ -31,6 +31,9 @@
>  
>  #include 
>  #include 
> +#ifndef _WIN32
> +#include 
> +#endif

Same as above: Why?

> diff --git a/src/xcb_in.c b/src/xcb_in.c
> index 8a7af92..ac4d284 100644
> --- a/src/xcb_in.c
> +++ b/src/xcb_in.c
> @@ -36,16 +36,17 @@
>  #include 
>  #include 
>  
> +#ifndef _WIN32
> +#include 
> +#include 
> +#endif
> +
>  #include "xcb.h"
>  #include "xcbext.h"
>  #include "xcbint.h"
>  #if USE_POLL
>  #include 
>  #endif
> -#ifndef _WIN32
> -#include 
> -#include 
> -#endif

I think that I must be missing something here...

If there is a reason for this, I'd like to have it in the commit message. If
this is just supposed to be clean up, it should be split out into its own
commit. If this is just needed for xcbint.h, then xcbint.h should include those
headers.

> diff --git a/src/xcb_list.c b/src/xcb_list.c
> index 129540b..075c535 100644
> --- a/src/xcb_list.c
> +++ b/src/xcb_list.c
> @@ -30,6 +30,9 @@
>  #endif
>  
>  #include 
> +#ifndef _WIN32
> +#include 
> +#endif
>  
>  #include "xcb.h"
>  #include "xcbint.h"
> diff --git a/src/xcb_out.c b/src/xcb_out.c
> index 429fa99..ed8458c 100644
> --- a/src/xcb_out.c
> +++ b/src/xcb_out.c
> @@ -33,6 +33,9 @@
>  #include 
>  #include 
>  #include 
> +#ifndef _WIN32
> +#include 
> +#endif
>  
>  #include "xcb.h"
>  #include "xcbext.h"
> @@ -263,6 +266,22 @@ unsigned int xcb_send_request(xcb_connection_t *c, int 
> flags, struct iovec *vect
>  return request;
>  }
>  
> +void
> +xcb_send_fd(xcb_connection_t *c, int fd)
> +{
> +#if HAVE_SENDMSG

  if (c->has_error)
return;

This is needed before the pthread_mutex_lock(), because if
xcb_connect_to_display() fails, it returns a pointer to an integer. The
has_error 'field' is the only thing that might be accessed for such a connection
object.

> +pthread_mutex_lock(&c->iolock);
> +while (c->out.out_fd.nfd == XCB_MAX_PASS_FD) {
> + _xcb_out_flush_to(c, c->out.request);
> + if (c->has_error)
> + break;
> +}
> +if (!c->has_error)
> + c->out.out_fd.fd[c->out.out_fd.nfd++] = fd;
> +pthread_mutex_unlock(&c->iolock);
> +#endif
> +}

So, xcb_send_fd() ha

Re: [Xcb] [PATCH 5/7] Add event queue splitting

[Uli Schlachter]

Hi,

On 07.11.2013 04:09, Keith Packard wrote:
> Peter Harris  writes:
> 
>> Dude. It's not getting NAKed, it's getting "Woah, this is out of left
>> field, this is the first I've ever heard of it, can we please have a
>> couple of days to think about it before we ACK it?"ed.
> 
> Yeah, I've been running this code for months now and have mentioned it
> when presenting DRI3000, but didn't ever bring it directly to the XCB
> list because I implemented precisely what we'd already agreed to in
> principle -- the notion of a simple event filtering mechanism that split
> events off into separate queues
> 
> I didn't realize until recently that I *hadn't* even sent the patches to
> the list though; DRI3000 has so many modules involved. I have posted
> links to the work since last January, and to my knowledge, no-one has
> ever looked at any of the code in question, so XCB is at least not alone
> in that regard.

Quite a convincing argument... :-P

> In any case, I'd like to implore all of you to consider a little
> deadline problem that I have today:
> 
>  * The Mesa freeze is Friday.

And this comes up on the xcb list two days before this deadline and only does so
accidentally...?

>  * All of the DRI3/Present code has been reviewed and is ready to be
>merged to master for that freeze.
> 
>  * However, it cannot be merged until a released version of XCB with
>these APIs is available.

Sorry, but this deadline might be a little close.

If this patch does get in, I am OK with cherry-picking these patches into a
branch and doing a release from that. But the general consensus with the whole
XKB (and more) situation seems to be that master is not in a releasable state.

>  * XCB is blocking the availability of a significant improvement to
>Mesa.
> 
> If we miss that freeze, DRI3/Present and the opportunity for GL support
> in XCB will languish for at least another six months. And that would be
> a shame.
> 
> The key to understanding these changes is:
> 
>  1) The notion of separate event queues with event filters to driver
> events into each queue was agreed to by several XCB developers a
> long time ago.

OK, I don't have a problem with this going in if it gets ACK'd by the "right
people" (whoever that is exactly). Since I will be offline, someone else would
then need to be convinced to do a new release based on 1.9.1 + these patches.

>  2) This design has the separate event queue APIs and implementation
> matching the requirements laid out informally in the historical
> discussions.
> 
>  3) Precisely how event filtering should work was never really worked
> out. I suspect the general notion would have been some complicated
> offset/mask/match algebra to allow selection based on fields within
> the event though.
> 
>  4) I constructed the simplest possible matching system which performs
> precisely the match needed for the Present extension, with the
> notion that a more complicated matching API should wait until we
> had actual requirements for that
> 
>  5) Adding new APIs to construct more complicated filters will not
> affect any public APIs offered in this patch.
> 
>  6) GLX is the last major X API which cannot be supported in XCB; this
> fixes that, so even if we end up with a dead-end API that is only
> good for this one thing, it will still be valuable enough to support
> forever (or, for at least as long as we're running X on the desktop,
> which appears to be functionally equivalent)

That would be great, yeah.

Uli

P.S.: Subscribed to xorg-devel just so that my mails don't end up in the
moderation queue...
-- 
A normal person is just someone you don't know well enough yet.
 - Nettie Wiebe
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Xcb] [PATCH 5/7] Add event queue splitting

[Uli Schlachter]

Hi,

On 06.11.2013 20:34, Peter Harris wrote:
> On 2013-11-05 19:41, Keith Packard wrote:
[...]
>> diff --git a/src/xcb.h b/src/xcb.h
>> index f7dc6af..f99e677 100644
>> --- a/src/xcb.h
>> +++ b/src/xcb.h
>> @@ -138,23 +138,6 @@ typedef struct {
>>  } xcb_generic_event_t;
>>  
>>  /**
>> - * @brief GE event
>> - *
>> - * An event as sent by the XGE extension. The length field specifies the
>> - * number of 4-byte blocks trailing the struct.
>> - */
>> -typedef struct {
>> -uint8_t  response_type;  /**< Type of the response */
>> -uint8_t  pad0;   /**< Padding */
>> -uint16_t sequence;   /**< Sequence number */
>> -uint32_t length;
>> -uint16_t event_type;
>> -uint16_t pad1;
>> -uint32_t pad[5]; /**< Padding */
>> -uint32_t full_sequence;  /**< full sequence */
>> -} xcb_ge_event_t;
>> -
>> -/**
> 
> This hunk doesn't appear to be related to event queue splitting. Should
> it be in a separate commit?

Thanks for CC'ing the xcb mailing list. I can see why this ended up on
xorg-devel (xcb being the only "sub-project" with its own list), but it would
still be nice to have patches for libxcb or xcb-proto on the xcb mailing list.

With google I found the patches in question and looked at them quickly. My first
impression is that the xcb_*_special_event() functions might need way better
documentation.

Also, this adds public API that allows to "hide" XGE events from the "normal"
APIs for getting events. Since this seems to be written just with present in
mind, I can see why it is done like this, but if we really want to go down that
road, I don't see why this should be XGE-specific.

Since this is public API, we should IMHO think thrice before settling on 
something.

For all the other patches, I am too lazy to get them out of the online archive.
I would count that as a NAK as long as it is understood that I don't have any
veto powers. ;-)

Uli
-- 
"Why make things difficult, when it is possible to make them cryptic
and totally illogical, with just a little bit more effort?" -- A. P. J.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel