Re: [PATCH xserver] modesetting: add missing ifdef GLAMOR

[Mihail Konev]

This seems to have been merged shortly after 1.19 RC1, i.e. non-critical
bugs deadline.

So it is a post-deadline fix for critical regression which occured in a
non-critical fix (which happened to be merged too-close to crit line).

___
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 libX11] Don't rebuild ks_tables.h if nothing changed.

[Martin Natano]

On Sat, Oct 08, 2016 at 09:45:09PM +0200, Matthieu Herrb wrote:
> 
> It looks good to me. 2 little things though:
> 
> - you should use 'git commit -s' to generate a Signed-off-by: field in
>   the commit message

See the updated diff below.


> 
> - the reason why this patch is needed is a limitation of BSD make
>   GNU make doesn't trigger the extra rebuild during make install.
> 
> PS: I tend to consider the BSD make behaviour as a bug, but no one ever
> cared to fix it :(

Curious, What's the bug? The force target is not marked phony, so it's
always out-of-date due to the eponymous file not existing. Then force
out-of-date -> rebuild makekeys, makekeys newer than ks_tables.h ->
rebuild ks_tables.h. What should the behaviour be instead?

natano


From 75d5e9b763069310cb2b0d0bac2a49175029449a Mon Sep 17 00:00:00 2001
From: Martin Natano 
Date: Sat, 8 Oct 2016 19:57:50 +0200
Subject: [PATCH] Don't rebuild ks_tables.h if nothing changed.

ks_tables.h is always considered out of date due to the forced rebuild
of the makekeys util. This means the file is also rebuilt during 'make
install', which is usually performed as root, which can to lead
permission problems later on.

Signed-off-by: Martin Natano 
---
 src/Makefile.am | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 15de59b..f8c476d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -415,7 +415,5 @@ ks_tables.h: $(KEYSYMDEFS) 
$(top_builddir)/src/util/makekeys$(EXEEXT)
$(top_builddir)/src/util/makekeys $(KEYSYMDEFS) > ks_tables_h
mv ks_tables_h $@
 
-$(top_builddir)/src/util/makekeys$(EXEEXT): force
+$(top_builddir)/src/util/makekeys$(EXEEXT): $(top_builddir)/src/util/makekeys.c
cd util && $(MAKE)
-
-force:
-- 
2.9.3

___
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 libX11] Don't rebuild ks_tables.h if nothing changed.

[Martin Natano]

This is my first patch to X.Org, so please tell me if there's something
wrong with the submission or the patch itself.

natano


From e3601d791790ee0f1d0979e4d2a3852c390cd758 Mon Sep 17 00:00:00 2001
From: Martin Natano 
Date: Sat, 8 Oct 2016 19:57:50 +0200
Subject: [PATCH] Don't rebuild ks_tables.h if nothing changed.

ks_tables.h is always considered out of date due to the forced rebuild
of the makekeys util. This means the file is also rebuilt during 'make
install', which is usually performed as root, which can to lead
permission problems later on.
---
 src/Makefile.am | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 15de59b..f8c476d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -415,7 +415,5 @@ ks_tables.h: $(KEYSYMDEFS) 
$(top_builddir)/src/util/makekeys$(EXEEXT)
$(top_builddir)/src/util/makekeys $(KEYSYMDEFS) > ks_tables_h
mv ks_tables_h $@
 
-$(top_builddir)/src/util/makekeys$(EXEEXT): force
+$(top_builddir)/src/util/makekeys$(EXEEXT): $(top_builddir)/src/util/makekeys.c
cd util && $(MAKE)
-
-force:
-- 
2.9.3

___
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] modesetting: add missing ifdef GLAMOR

[Mihail Konev]

This fixes --disable-glamor failing to build.

Regressed-in: e8695100b17b758359fc4897dbe995231ed224fc
Signed-off-by: Mihail Konev 
---
 hw/xfree86/drivers/modesetting/driver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/xfree86/drivers/modesetting/driver.c 
b/hw/xfree86/drivers/modesetting/driver.c
index 216388f0a36c..3da69a3962a0 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -594,6 +594,7 @@ redisplay_dirty(ScreenPtr screen, PixmapDirtyUpdatePtr 
dirty, int *timeout)
 PixmapSyncDirtyHelper(dirty);
 
 if (!screen->isGPU) {
+#ifdef GLAMOR
 /*
  * When copying from the master framebuffer to the shared pixmap,
  * we must ensure the copy is complete before the slave starts a
@@ -602,6 +603,7 @@ redisplay_dirty(ScreenPtr screen, PixmapDirtyUpdatePtr 
dirty, int *timeout)
  */
 if (ms->drmmode.glamor)
 glamor_finish(screen);
+#endif
 /* Ensure the slave processes the damage immediately */
 if (timeout)
 *timeout = 0;
-- 
2.9.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

[PATCH] Add git format.subjectPrefix to modules

[Mihail Konev]

Hello.

Xserver's autogen.sh checks for format.subjectPrefix git
preference to be defined, and sets a default in case there is none.

Other modules are missing this feature.

To generate the patches,

In xorg/util/modular directory, cloned all the modules
(thanks to build.sh -L).
Ran the attached subject_all.sh script.
Changed to patches dir ./subjectPrefix_patches/ it created.
Verified the patches with "grep changed * | grep -v ' 3 insert'".
Ran the attached all.sh script.

Attached is the all.tar.xz result.
Below is a sample for xcb-proto.

> From 13713caf3194581501fa891e2da1c1c33cd47693 Mon Sep 17 00:00:00 2001
> From: Mihail Konev 
> Date: Mon, 10 Oct 2016 15:01:11 +
> Subject: [PATCH xcb/proto] add git format.subjectPrefix to autogen.sh
> 
> Signed-off-by: Mihail Konev 
> ---
>  autogen.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/autogen.sh b/autogen.sh
> index fc34bd55c443..78d554b5cdc0 100755
> --- a/autogen.sh
> +++ b/autogen.sh
> @@ -9,6 +9,9 @@ cd $srcdir
>  autoreconf -v --install || exit 1
>  cd $ORIGDIR || exit $?
>  
> +git config --local --get format.subjectPrefix >/dev/null 2>&1 ||
> +git config --local format.subjectPrefix "PATCH xcb/proto"
> +
>  if test -z "$NOCONFIGURE"; then
>  $srcdir/configure "$@"
>  fi
> -- 
> 2.9.2
> 

- Mihail


subject_all.sh
Description: Bourne shell script


all.sh
Description: Bourne shell script


all.tar.xz
Description: application/xz
___
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] inputthread: Fix inputthread not listening if a fd gets re-added immediately after removal

[Mihail Konev]

Hello.

On Wed,  5 Oct 2016 14:45:20 +0200, Hans de Goede wrote:
> When a fd is removed dev->state gets set to device_state_removed,
> if the fd then gets re-added before InputThreadDoWork() deals with
> the removal, the InputThreadDevice struct gets reused, but its
> state would stay device_state_removed, so it would still get removed
> on the first InputThreadDoWork() run, resulting in a non functioning
> input device.
> 
> This commit fixes this by (re-)setting dev->state to device_state_running
> when a InputThreadDevice struct gets reused.
> 
> This fixes input devices sometimes no longer working after a vt-switch.
> 
> Signed-off-by: Hans de Goede 
> ---
>  os/inputthread.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/os/inputthread.c b/os/inputthread.c
> index 6aa0a9c..ab1559f 100644
> --- a/os/inputthread.c
> +++ b/os/inputthread.c
> @@ -206,6 +206,8 @@ InputThreadRegisterDev(int fd,
>  if (dev) {
>  dev->readInputProc = readInputProc;
>  dev->readInputArgs = readInputArgs;
> +/* Override possible unhandled state == device_state_removed */
> +dev->state = device_state_running;
>  } else {
>  dev = calloc(1, sizeof(InputThreadDevice));
>  if (dev == NULL) {
> --
> 2.9.3
> 

On 6 Oct 2016, Hans de Goede wrote:
> On 24-09-16 19:55, Mihail Konev wrote:
> > <..>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97880
> 
> Thank you for the patch and you're right that there is an issue here.
> 
> I've posted a much simpler fix yesterday, and that has been
> favorably reviewed, so I think we're going to go with that fix
> instead, <..>
> 

Applied on top of 9e00cf0f75f286ec0e8137d91ba80ef7ba72ab2a , the patch does not
solve the bug #97880 for me.

Note: ./configure --disable-glamor is broken somewhere after the ade315386
(Require xproto 7.0.31), so be sure to --enable-glamor.

On 9 Oct 2016, Hans de Goede wrote:
> So you've tried my patch, with your patch reverted and it
> does not work for you? Strange as I was hitting the exact
> same problem and it did work for me.
> 

The difference in problem is, however, that in my case result is
consistent - keyboard either works or not all the time, not "sometimes".
Mouse is moving alright.

First, patch was applied on top of fc1c358b9 (1.19 RC1).
It did not work, so log1.diff was made, applied,
and log1 produced (both attached).
Note: log1 is (fully) commented inline.

Rebasing on top of 97a8353ec (Fix id in error) did not help.

At this point (log1), nol_e.diff gives:
- nol_e.log1
- bug being present

Applying nol_d.diff to log1 gives:
- nol_d.log1
- bug being gone

Manually applied log1.diff to master 97a8353ec (gave log1m.diff).

At this point (log1m) nol_e.diff gives:
- nol_e.log1m
- bug being present
Mistake in bugzilla bug description:
this shows that not locking EnableDevice is not a sufficient workaround.

Applying nol_d.diff to log1m gives:
- nol_d.log1m
- bug being gone
This is what is right in bugzilla description:
not locking DisableDevice is a sufficient workaround.

As I see it,

The patch assumes no device state to be maintained except in the input thread 
dev
list.  The dev->state is just a flag to signal the input thread to process
input state change.  When it is device_state_running, input thread would not do
any processing upon the dev list element (which through the dev->fd corresponds
to /dev/input/eventN).

Before commit 52d6a1e832a5e62289dd4f32824ae16a78dfd7e8, input thread would
process events before the input_lock is called in the xf86VTLeave,
(that is, as-soon-as xf86DisableDeviceForVTSwitch sets the
`dev->state = device_state_removed`).

Starting with the commit 52d6a1e832a5e62289dd4f32824ae16a78dfd7e8, input_lock
is called in the xf86{Enable,Disable}InputDeviceForVTSwitch (actually in the
dix {Enable,Disable}Device, but xf86* just wrap them).  As the first thing the
main thread does after calling all xf86Disable..  is to call input_lock
(without an input_unlock until VT is switched back), input thread has almost no
chance to process `dev->state == device_state_removed` for the last device.

Note: The chance could be given by inserting something relatively long-running,
like LogMessageVerb(X_INFO, 0, "[xf86VTLeave] doing input_lock...\n").  Such a
line is commented out in log1.patch; before commenting, the bug was not seen.

Note: Input thread would not process releases after VT switch in any case,
as input_lock is released by main thread only at the very end of xf86VTEnter.
(This is why not locking the EnableDevice does not help - lock is already held).

So when the main thread does input_unlock at the end of xf86VTEnter,
the input thread would see:
- N-1 elements `dev->state == device_state_added`
- One last element `dev-state == device_state_removed`
  (or `device_state_running` with the above patch applied)

For N-1 elements the input thread would call the proper attach procedures
(that is, call into xf86-input-evdev, which possibly calls into OS).
For 

Re: Request changes in Compose.pre

[Mihail Konev]

On Wed Oct 5 09:40:28 UTC 2016, Victor V. Kustov wrote:
> Sorry for noise, but I'm discouraged by silence. Maybe I need send
> patch by another address or maybe I doing wrong something... Please
> give me a tips how I may do it correctly.

There are the steps supposed to be performed.

http://x.org -> DeveloperStart -> Building X Window System

First, clone the root repo.
$ cd ~
$ git clone git://anongit.freedesktop.org/git/xorg/util/modular xorg
$ cd xorg
$ mkdir b

List the subprojects.
$ sh build.sh -L

Ask it to clone (and build) the lib/libX11 (as the Compose.pre belongs to it).
No problem if build fails.
$ sh build.sh --clone -o lib/libX11 $(pwd)/b
Or clone manually:
$ mkdir lib
$ git clone git://anongit.freedesktop.org/git/xorg/lib/libX11 lib/libX11

Now let's patch it.
$ cd ~/xorg/lib/libX11

Prepare the repository.
$ git checkout -b ruble_sign

Locate the file of interest.
$ find -iname "*compose*"

Apply the changes.
$ vim nls/en_US.UTF-8/Compose.pre

Save them into repository.
$ git status
$ git add nls
$ git diff --staged
$ export EDITOR=vim
$ git commit -e

Follow the Xorg guidelines.
$ git commit --amend --signoff
$ git config --local format.subjectPrefix "PATCH libX11"

Prepare the email.
$ git format-patch HEAD^

Example result below.

---
 nls/en_US.UTF-8/Compose.pre | 1 +
 1 file changed, 1 insertion(+)

diff --git a/nls/en_US.UTF-8/Compose.pre b/nls/en_US.UTF-8/Compose.pre
index adc24fb5b5c2..9ea2235b4079 100644
--- a/nls/en_US.UTF-8/Compose.pre
+++ b/nls/en_US.UTF-8/Compose.pre
@@ -190,6 +190,7 @@ XCOMM "₪" U20aa NEW SHEQEL SIGN
 : "€"   EuroSign # EURO SIGN
 : "€"   EuroSign # EURO SIGN
 : "€"   EuroSign # EURO SIGN
+XCOMM : "₽"   U20bd # RUBLE-CURRENCY SIGN
 XCOMM "₭" U20ad KIP SIGN
 XCOMM "₮" U20ae TUGRIK SIGN
 XCOMM "₯" U20af DRACHMA SIGN
-- 
2.9.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 3/3] glx: Initialize glx even if there are currently no screens attached

[Jeremy Huddleston Sequoia]

> On Oct 10, 2016, at 10:16, Adam Jackson  wrote:
> 
> On Sun, 2016-10-09 at 12:51 -0700, Jeremy Huddleston Sequoia wrote:
>> Failure to do so causes an overvlow in glxClientCallback
> 
> This patch makes no sense at all. glxClientCallback is only added to
> the call chain _after_ we check for a GL-capable visual. If that check
> is preventing GLX from initializing, then it also prevents
> glxClientCallback being called.
> 
> More to the point, the check is still correct, Mesa still needs a TC/DC
> visual to work. I suppose non-Mesa systems might not have that
> property, but a) OSX doesn't support pseudocolor GL rendering I am like
> 99% sure and b) that just means the check belongs in screen init
> instead of being deleted entirely.
> 
> What are you trying to do here?

As I mentioned in the other thread, I agree that this shouldn't do anything wrt 
to ASan report, so I'm a bit confused by that and will dig into it.

However, from a correctness standpoint, I think this is still the right 
approach.

Yes, GLX requires a GL-capable visual in order to work, but that doesn't mean 
that it needs to be present at the time that the extension is initialized.  We 
be able to start with 0 screens attached and then treat adding #1 the same as 
we would adding #2.

I agree with you that these should probably be moved into RRScreenInit / 
__glXScreenInit and will make such a change in the next round.  I want to spend 
some more time trying to figure out what was going on with the ASan report 
because indeed it doesn't make sense that this should cause it to go away.

--Jeremy

___
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 3/3] glx: Initialize glx even if there are currently no screens attached

[Adam Jackson]

On Sun, 2016-10-09 at 12:51 -0700, Jeremy Huddleston Sequoia wrote:
> Failure to do so causes an overvlow in glxClientCallback

This patch makes no sense at all. glxClientCallback is only added to
the call chain _after_ we check for a GL-capable visual. If that check
is preventing GLX from initializing, then it also prevents
glxClientCallback being called.

More to the point, the check is still correct, Mesa still needs a TC/DC
visual to work. I suppose non-Mesa systems might not have that
property, but a) OSX doesn't support pseudocolor GL rendering I am like
99% sure and b) that just means the check belongs in screen init
instead of being deleted entirely.

What are you trying to do here?

- ajax
___
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 2/3] randr: Initialize RandR even if there are currently no screens attached

[Jeremy Huddleston Sequoia]

> On Oct 10, 2016, at 03:33, Emil Velikov  wrote:
> 
> Hi Jeremy,
> 
> On 9 October 2016 at 20:51, Jeremy Huddleston Sequoia
>  wrote:
>> Failure to do so causes an overvlow in RRClientCallback().
>> 
> s/overvlow/overflow/

Doh, corrected the typo, thanks.

> Perhaps a slightly silly question:
> How can one end up in the callback if we haven't executed
> AddCallback(, fooCallback... ?

Not silly at all, and actually quite pointed.  The same basic question applies 
to this and the third patch in the series.

From a correctness standpoint, it makes sense that GLX and RandR should 
initialize even when the first display hasn't yet been configured.  They should 
handle attachment of the first display in much the same way they'd handle 
attaching the second or third display.

Looking at the actual change, I agree with you that one should not expect the 
change to have any impact on the reported problem because the removed 
early-exit was before AddCallback() in both cases.  I developed these changes 
against the commit that introduced the regression 
(30ac7567980a1eb79d084a63e0e74e1d9a3af673), so I just took a look at the tree 
back at that time to see if maybe something else had changed between then and 
current master to explain this.  Nothing obvious stands out, which begs the 
question of why this had any impact whatsoever on this issue.

I'm curious and will trace through it more when I get some cycles to figure out 
what's actually going on there.

In the mean time, this is still valid for correctness reasons, so I'd like to 
get some feedback on it from that angle while I try to figure out what was 
really going on in those reported overflows.

Thanks,
Jeremy



smime.p7s
Description: S/MIME cryptographic signature
___
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 3/3] configure.ac: remove --enable-aiglx option

[Jon Turney]

On 09/10/2016 15:42, Emil Velikov wrote:

On Friday, 7 October 2016, Jon Turney wrote:


That's not quite enough, as building glxdri2.c also requires dri2proto
headers.

At the moment, configure.ac only requires dri2proto when --enable-dri2
turns on.

So either that needs to be made unconditional, or building glxdri2.c made
conditional on DRI2 (untested patch attached)

You're correct. Wrapping it in DRI2 conditional is a good idea.


Note creating an empty (no sources or static libs) library is likely to
cause problems. Just use the form prior to my patch ?


Yes, that seems possible.  Updated patch attached.


From 42f74bb44190be06b9630dfcaae48b27533a28cd Mon Sep 17 00:00:00 2001
From: Jon Turney 
Date: Thu, 6 Oct 2016 22:13:07 +0100
Subject: [PATCH xserver] glx/dri2: Don't build DRI loader if DRI2 isn't
 enabled

This partially reverts 501d8e2b.
---
 glx/Makefile.am| 11 ---
 hw/xfree86/dixmods/Makefile.am |  2 ++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/glx/Makefile.am b/glx/Makefile.am
index fc0b76a..60bd84c 100644
--- a/glx/Makefile.am
+++ b/glx/Makefile.am
@@ -1,4 +1,8 @@
-noinst_LTLIBRARIES = libglx.la libglxdri.la
+if DRI2
+GLXDRI_LIBRARY = libglxdri.la
+endif
+
+noinst_LTLIBRARIES = libglx.la $(GLXDRI_LIBRARY)
 
 AM_CFLAGS = \
@DIX_CFLAGS@ \
@@ -16,11 +20,10 @@ AM_CPPFLAGS = \
-I$(top_srcdir)/hw/xfree86/os-support/bus \
-I$(top_srcdir)/hw/xfree86/common \
-I$(top_srcdir)/hw/xfree86/dri \
+   -I$(top_srcdir)/hw/xfree86/dri2
-I$(top_srcdir)/mi \
-I$(top_srcdir)/present
 
-AM_CPPFLAGS += -I$(top_srcdir)/hw/xfree86/dri2
-
 indirect_sources = \
indirect_dispatch.c \
indirect_dispatch.h \
@@ -33,7 +36,9 @@ indirect_sources =\
indirect_table.c
 
 libglxdri_la_SOURCES =
+if DRI2
 libglxdri_la_SOURCES += glxdri2.c
+endif
 
 libglxdri_la_LIBADD = $(DLOPEN_LIBS)
 
diff --git a/hw/xfree86/dixmods/Makefile.am b/hw/xfree86/dixmods/Makefile.am
index be43e8f..d534c78 100644
--- a/hw/xfree86/dixmods/Makefile.am
+++ b/hw/xfree86/dixmods/Makefile.am
@@ -29,10 +29,12 @@ libwfb_la_CFLAGS = $(AM_CFLAGS) -DFB_ACCESS_WRAPPER
 
 libglx_la_LDFLAGS = -module -avoid-version $(LD_NO_UNDEFINED_FLAG)
 libglx_la_LIBADD = $(top_builddir)/glx/libglx.la $(GLX_SYS_LIBS)
+if DRI2
 libglx_la_LIBADD += $(top_builddir)/glx/libglxdri.la
 if NO_UNDEFINED
 libglx_la_LIBADD += $(LIBDRM_LIBS) $(PIXMAN_LIBS)
 endif
+endif
 libglx_la_SOURCES = glxmodule.c
 
 libshadow_la_LDFLAGS = -module -avoid-version $(LD_NO_UNDEFINED_FLAG)
-- 
2.8.3

___
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

[xserver PULL for 1.19] Various fixes

[Hans de Goede]

Hi Adam, Keith,

Here is a pull-req with 2 prime hw cursor fixes from Michel Dänzer,
reviewed by me and 1 fix from me reviewed by Keith.

The following changes since commit 97a8353ec1192d8d3bd2ebb99e5687cb91427e09:

  Fix id in error when resource does not exist (2016-10-06 14:50:42 -0400)

are available in the git repository at:

  git://people.freedesktop.org/~jwrdegoede/xserver for-server-1.19

for you to fetch changes up to 6979eb819e436416029a8ff32e7d356322939898:

  xf86Cursor: Take the input lock in xf86Set/MoveCursor (2016-10-10 11:14:56 
+0200)


Hans de Goede (1):
  inputthread: Fix inputthread not listening if a fd gets re-added 
immediately after removal

Michel Dänzer (2):
  xf86Cursor: Use PRIME master xf86CursorScreenRec::HotX/Y for slaves
  xf86Cursor: Take the input lock in xf86Set/MoveCursor

 hw/xfree86/ramdac/xf86HWCurs.c | 37 ++---
 os/inputthread.c   |  2 ++
 2 files changed, 32 insertions(+), 7 deletions(-)

Regards,

Hans
___
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 xf86-input-libinput] Fix crash when using threaded input and the first device goes away

[Hans de Goede]

Hi,

On 10/10/2016 11:10 AM, Emil Velikov wrote:

Hi Hans,

On Wednesday, 5 October 2016, Hans de Goede > wrote:

When the xserver uses threaded input, it keeps a pointer to the InputInfo
passed into xf86AddEnabledDevice and calls pointer->read_input on events.

But when the first enabled device goes away the pInfo we've passed into
xf86AddEnabledDevice gets freed and eventually pInfo->read_input gets
overwritten (or pInfo points to unmapped memory) leading to a segfault.

This commit fixes this by replacing the pInfo passed into
xf86AddEnabledDevice with a pointer to a global InputInfo stored inside
the driver_context struct.

Signed-off-by: Hans de Goede 
---
 src/xf86libinput.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/xf86libinput.c b/src/xf86libinput.c
index 21f87f5..485e212 100644
--- a/src/xf86libinput.c
+++ b/src/xf86libinput.c
@@ -86,6 +86,7 @@

 struct xf86libinput_driver {
struct libinput *libinput;
+   struct _InputInfoRec InputInfo;
int device_enabled_count;
 };

@@ -582,7 +583,17 @@ xf86libinput_on(DeviceIntPtr dev)

if (driver_context.device_enabled_count == 0) {
 #if HAVE_THREADED_INPUT
-   xf86AddEnabledDevice(pInfo);
+   /*
+* The xserver keeps a pointer to the InputInfo passed into
+* xf86AddEnabledDevice and calls pointer->read_input on
+* events. Thus we cannot simply pass in our current pInfo
+* as that will be deleted when the current input device 
gets
+* unplugged. Instead pass in a pointer to a global
+* InputInfo inside the driver_context.
+*/
+   driver_context.InputInfo.fd = pInfo->fd;

Reading the above comment makes me wonder about the lifetime of the fd. I take 
it that we're not leaking it atm ?


The fd here actually is libinput's epoll fd, which is shared by all the devices
and gets closed when we destroy the libinput context which we do already when 
the
last libinput driven device gets removed.


+   driver_context.InputInfo.read_input = pInfo->read_input;
+   xf86AddEnabledDevice(_context.InputInfo);
 #else
/* Can't use xf86AddEnabledDevice on an epollfd */
AddEnabledDevice(pInfo->fd);

Can we use the same storage for the !HAVE_THREADED_INPUT code paths ?


I've not looked closely at the !threaded code paths, but AFAIK the non threaded
paths take just a fd; and then later lookup the pInfo in the list of devices,
so in that case the pInfo must be a real pInfo.




@@ -606,7 +617,7 @@ xf86libinput_off(DeviceIntPtr dev)

if (--driver_context.device_enabled_count == 0) {
 #if HAVE_THREADED_INPUT
-   xf86RemoveEnabledDevice(pInfo);
+   xf86RemoveEnabledDevice(_context.InputInfo);
 #else
RemoveEnabledDevice(pInfo->fd);
 #endif
@@ -1923,7 +1934,7 @@ out:
 }

 static void
-xf86libinput_read_input(InputInfoPtr pInfo)
+xf86libinput_read_input(InputInfoPtr do_not_use)

Worth just dropping the argument and fixing the caller(s)?


This is a callback function called by the server, so we cannot just
change the signature; and other input drivers are likely to actually
use the argument.

Regards,

Hans
___
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 xserver] os/xdmcp: Honour -once when session is dead

[walter harms]

looks good to me (and hope that helps)

reviewed-by: wharms 

re,
 wh

Am 30.09.2016 14:05, schrieb Daniel Martin:
> Terminate a dead session when -once was passed. Don't restart it.
> 
> Signed-off-by: Daniel Martin 
> ---
>  os/xdmcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/os/xdmcp.c b/os/xdmcp.c
> index 906c959..7aeb393 100644
> --- a/os/xdmcp.c
> +++ b/os/xdmcp.c
> @@ -797,7 +797,7 @@ XdmcpDeadSession(const char *reason)
>  ErrorF("XDM: %s, declaring session dead\n", reason);
>  state = XDM_INIT_STATE;
>  isItTimeToYield = TRUE;
> -dispatchException |= DE_RESET;
> +dispatchException |= (OneSession ? DE_TERMINATE : DE_RESET);
>  TimerCancel(xdmcp_timer);
>  timeOutRtx = 0;
>  send_packet();
___
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 libXi] SizeClassInfo can return 0 even without an error

[walter harms]

Am 10.10.2016 12:24, schrieb Emil Velikov:
> On 9 October 2016 at 21:31, Niels Ole Salscheider
>  wrote:
>> Hi Emil,
>>
>> On Sunday, 9 October 2016, 15:34:28 CEST, Emil Velikov wrote:
>>> Hi Niels,
>>>
>>> On Friday, 7 October 2016, Niels Ole Salscheider <
>>>
>>> niels_...@salscheider-online.de> wrote:
 Catch the error case separately. This fixes a few crashes on my computer.

 Signed-off-by: Niels Ole Salscheider >
 ---

  src/XListDev.c | 21 ++---
  1 file changed, 10 insertions(+), 11 deletions(-)

 diff --git a/src/XListDev.c b/src/XListDev.c
 index f850cd0..d0c6bf2 100644
 --- a/src/XListDev.c
 +++ b/src/XListDev.c
 @@ -73,27 +73,27 @@ static int pad_to_xid(int base_size)

  return ((base_size + padsize - 1)/padsize) * padsize;

  }

 -static size_t
 -SizeClassInfo(xAnyClassPtr *any, size_t len, int num_classes)
 +static int
 +SizeClassInfo(xAnyClassPtr *any, size_t len, int num_classes, size_t
 *size)

  {

 -int size = 0;

  int j;

 +*size = 0;
>>>
>>> No function should alter the contents of the arguments in case of an error.
>>> For your other libXi patch one might want to fix the callers, if applicable.
>>>
>>> If possible please mention a bug report/link or a bit more about how you
>>> hit this. Wondering how it has gone unnoticed for so long.
>>
>> I encountered the bug in chromium and all users of it, including all
>> applications that use QtWebEngine. I now hit the error path because of the 
>> bug
>> that is fixed by this patch.
>>
>> Chromium crashes in the following lines: https://chromium.googlesource.com/
>> chromium/src/+/master/ui/events/devices/x11/device_data_manager_x11.cc#246
>> Here, GetXDeviceList calls XListInputDevices:
>> https://chromium.googlesource.com/chromium/src/+/master/ui/events/devices/x11/
>> device_list_cache_x11.cc#53
>>
>> The chromium implementation is only correct if ndevices is set to 0 in the
>> error case since it does not check if a null pointer is returned. I was not
>> sure if it is supposed to do the latter since the man page for
>> XListInputDevices doesn't mention it.
>>
> Eeek, the man page does not mention anything, indeed. So even if one
> updates/corrects the man page there'll likely be other users which
> depend on ndevices being 0.

but someone should fix the man-page. NTL it is common practice to assume
that variables are not proper initialized if the function that initialize
returns any error.

re,
 wh

> 
> That said, you can drop the odd(?) practise from this patch and use it
> only in the latter ? Please mention your findings (above) in the
> commit message/patch body.
> 
> Thanks
> Emil
> ___
> 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
___
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 xserver] os/xdmcp: Honour -once when session is dead

[Daniel Martin]

Imho this could be picked for 1.19. Anyone willing to give his Rb?

On 30 September 2016 at 14:05, Daniel Martin  wrote:
> Terminate a dead session when -once was passed. Don't restart it.
>
> Signed-off-by: Daniel Martin 
> ---
>  os/xdmcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/os/xdmcp.c b/os/xdmcp.c
> index 906c959..7aeb393 100644
> --- a/os/xdmcp.c
> +++ b/os/xdmcp.c
> @@ -797,7 +797,7 @@ XdmcpDeadSession(const char *reason)
>  ErrorF("XDM: %s, declaring session dead\n", reason);
>  state = XDM_INIT_STATE;
>  isItTimeToYield = TRUE;
> -dispatchException |= DE_RESET;
> +dispatchException |= (OneSession ? DE_TERMINATE : DE_RESET);
>  TimerCancel(xdmcp_timer);
>  timeOutRtx = 0;
>  send_packet();
> --
> 2.10.0
>
___
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 2/3] randr: Initialize RandR even if there are currently no screens attached

[Emil Velikov]

Hi Jeremy,

On 9 October 2016 at 20:51, Jeremy Huddleston Sequoia
 wrote:
> Failure to do so causes an overvlow in RRClientCallback().
>
s/overvlow/overflow/

Perhaps a slightly silly question:
How can one end up in the callback if we haven't executed
AddCallback(, fooCallback... ?

Regards,
Emil
___
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 libXi] SizeClassInfo can return 0 even without an error

[Emil Velikov]

On 9 October 2016 at 21:31, Niels Ole Salscheider
 wrote:
> Hi Emil,
>
> On Sunday, 9 October 2016, 15:34:28 CEST, Emil Velikov wrote:
>> Hi Niels,
>>
>> On Friday, 7 October 2016, Niels Ole Salscheider <
>>
>> niels_...@salscheider-online.de> wrote:
>> > Catch the error case separately. This fixes a few crashes on my computer.
>> >
>> > Signed-off-by: Niels Ole Salscheider > > >
>> > ---
>> >
>> >  src/XListDev.c | 21 ++---
>> >  1 file changed, 10 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/src/XListDev.c b/src/XListDev.c
>> > index f850cd0..d0c6bf2 100644
>> > --- a/src/XListDev.c
>> > +++ b/src/XListDev.c
>> > @@ -73,27 +73,27 @@ static int pad_to_xid(int base_size)
>> >
>> >  return ((base_size + padsize - 1)/padsize) * padsize;
>> >
>> >  }
>> >
>> > -static size_t
>> > -SizeClassInfo(xAnyClassPtr *any, size_t len, int num_classes)
>> > +static int
>> > +SizeClassInfo(xAnyClassPtr *any, size_t len, int num_classes, size_t
>> > *size)
>> >
>> >  {
>> >
>> > -int size = 0;
>> >
>> >  int j;
>> >
>> > +*size = 0;
>>
>> No function should alter the contents of the arguments in case of an error.
>> For your other libXi patch one might want to fix the callers, if applicable.
>>
>> If possible please mention a bug report/link or a bit more about how you
>> hit this. Wondering how it has gone unnoticed for so long.
>
> I encountered the bug in chromium and all users of it, including all
> applications that use QtWebEngine. I now hit the error path because of the bug
> that is fixed by this patch.
>
> Chromium crashes in the following lines: https://chromium.googlesource.com/
> chromium/src/+/master/ui/events/devices/x11/device_data_manager_x11.cc#246
> Here, GetXDeviceList calls XListInputDevices:
> https://chromium.googlesource.com/chromium/src/+/master/ui/events/devices/x11/
> device_list_cache_x11.cc#53
>
> The chromium implementation is only correct if ndevices is set to 0 in the
> error case since it does not check if a null pointer is returned. I was not
> sure if it is supposed to do the latter since the man page for
> XListInputDevices doesn't mention it.
>
Eeek, the man page does not mention anything, indeed. So even if one
updates/corrects the man page there'll likely be other users which
depend on ndevices being 0.

That said, you can drop the odd(?) practise from this patch and use it
only in the latter ? Please mention your findings (above) in the
commit message/patch body.

Thanks
Emil
___
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 1/3] os/connection: Improve abstraction for launchd secure sockets

[Emil Velikov]

On 9 October 2016 at 20:51, Jeremy Huddleston Sequoia
 wrote:
> This changes away from hard-coding the /tmp/launch-* path to now
> supporting a generic [.]
> format for $DISPLAY.
>
> cf-libxcb: d978a4f69b30b630f28d07f1003cf290284d24d8
>
> Signed-off-by: Jeremy Huddleston Sequoia 
> CC: Adam Jackson 
> ---
>  os/connection.c | 31 ++-
>  1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/os/connection.c b/os/connection.c
> index a901ebf..0d42184 100644
> --- a/os/connection.c
> +++ b/os/connection.c
> @@ -79,6 +79,8 @@ SOFTWARE.
>  #include 
>  #include 
>
> +#include 
> +
>  #ifndef WIN32
>  #include 
>
> @@ -1112,15 +1114,34 @@ MakeClientGrabPervious(ClientPtr client)
>  void
>  ListenOnOpenFD(int fd, int noxauth)
>  {
> -char port[256];
> +char port[PATH_MAX];
... = {0, }; // or port[0] = 0;

>  XtransConnInfo ciptr;
>  const char *display_env = getenv("DISPLAY");
>
> -if (display_env && (strncmp(display_env, "/tmp/launch", 11) == 0)) {
> -/* Make the path the launchd socket if our DISPLAY is set right */
> -strcpy(port, display_env);
> +/* First check if display_env matches a  socket>[.] scheme (eg: launchd) */
> +if (display_env && display_env[0] == '/') {
As-is the patch will accept badly formed DISPLAY and port will end up
garbage. Might be better to keep track of/override port instead.

> +struct stat sbuf;
> +
> +strlcpy(port, display_env, sizeof(port));
> +
> +/* If the path exists, we don't have do do anything else.
> + * If it doesn't, we need to check for a . to strip 
> off and recheck.
> + */
> +if (0 != stat(port, )) {
Nit: !stat(...)

> +char *dot = strrchr(port, '.');
> +if (dot) {
> +*dot = '\0';
> +
> +if (0 != stat(port, )) {
Ditto

> +display_env = NULL;
port[0] = 0;

> +}
> +} else {
> +display_env = NULL;
Ditto.

> +}
> +}
>  }
> -else {
> +
> +if (!display_env) {
if (!port[0]) {

Regards,
Emil
___
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 xf86-input-libinput] Fix crash when using threaded input and the first device goes away

[Emil Velikov]

Hi Hans,

On Wednesday, 5 October 2016, Hans de Goede  wrote:

> When the xserver uses threaded input, it keeps a pointer to the InputInfo
> passed into xf86AddEnabledDevice and calls pointer->read_input on events.
>
> But when the first enabled device goes away the pInfo we've passed into
> xf86AddEnabledDevice gets freed and eventually pInfo->read_input gets
> overwritten (or pInfo points to unmapped memory) leading to a segfault.
>
> This commit fixes this by replacing the pInfo passed into
> xf86AddEnabledDevice with a pointer to a global InputInfo stored inside
> the driver_context struct.
>
> Signed-off-by: Hans de Goede >
> ---
>  src/xf86libinput.c | 22 --
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/src/xf86libinput.c b/src/xf86libinput.c
> index 21f87f5..485e212 100644
> --- a/src/xf86libinput.c
> +++ b/src/xf86libinput.c
> @@ -86,6 +86,7 @@
>
>  struct xf86libinput_driver {
> struct libinput *libinput;
> +   struct _InputInfoRec InputInfo;
> int device_enabled_count;
>  };
>
> @@ -582,7 +583,17 @@ xf86libinput_on(DeviceIntPtr dev)
>
> if (driver_context.device_enabled_count == 0) {
>  #if HAVE_THREADED_INPUT
> -   xf86AddEnabledDevice(pInfo);
> +   /*
> +* The xserver keeps a pointer to the InputInfo passed into
> +* xf86AddEnabledDevice and calls pointer->read_input on
> +* events. Thus we cannot simply pass in our current pInfo
> +* as that will be deleted when the current input device
> gets
> +* unplugged. Instead pass in a pointer to a global
> +* InputInfo inside the driver_context.
> +*/
> +   driver_context.InputInfo.fd = pInfo->fd;

Reading the above comment makes me wonder about the lifetime of the fd. I
take it that we're not leaking it atm ?


> +   driver_context.InputInfo.read_input = pInfo->read_input;
> +   xf86AddEnabledDevice(_context.InputInfo);
>  #else
> /* Can't use xf86AddEnabledDevice on an epollfd */
> AddEnabledDevice(pInfo->fd);

Can we use the same storage for the !HAVE_THREADED_INPUT code paths ?


> @@ -606,7 +617,7 @@ xf86libinput_off(DeviceIntPtr dev)
>
> if (--driver_context.device_enabled_count == 0) {
>  #if HAVE_THREADED_INPUT
> -   xf86RemoveEnabledDevice(pInfo);
> +   xf86RemoveEnabledDevice(_context.InputInfo);
>  #else
> RemoveEnabledDevice(pInfo->fd);
>  #endif
> @@ -1923,7 +1934,7 @@ out:
>  }
>
>  static void
> -xf86libinput_read_input(InputInfoPtr pInfo)
> +xf86libinput_read_input(InputInfoPtr do_not_use)

Worth just dropping the argument and fixing the caller(s)?

Emil
___
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