Re: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[Pierre Ossman]

On 09/01/17 20:15, Hans de Goede wrote:


Yes I agree that would be better, Pierre, can you take care
of merging Alan's improved version ?



All done and available on master now. Thanks for fixing this.

Regards
--
Pierre Ossman   Software Development
Cendio AB   https://cendio.com
Teknikringen 8  https://twitter.com/ThinLinc
583 30 Linköpinghttps://facebook.com/ThinLinc
Phone: +46-13-214600https://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
___
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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[Hans de Goede]

Hi,

On 09-01-17 18:12, Alan Coopersmith wrote:

On 01/ 9/17 07:57 AM, Hans de Goede wrote:

 close(2);
+/* Avoid xserver >= 1.19's epoll-fd becoming fd 2 / stderr only to be
+   replaced by /dev/null by OsInit() because the pollfd is not
+   writable, breaking ospoll_wait(). */
+open("/dev/null", O_WRONLY);


Hopefully no other threads in the X server are opening files, but if any ever
do, it would be more reliable to do:

nullfd = open("/dev/null", O_WRONLY);
dup2(nullfd, 2);
close(nullfd);

and let dup2 atomically close the old stderr and clone nullfd to it.


Yes I agree that would be better, Pierre, can you take care
of merging Alan's improved version ?

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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[Keith Packard]

Alan Coopersmith  writes:

> On 01/ 9/17 07:57 AM, Hans de Goede wrote:
>>  close(2);
>> +/* Avoid xserver >= 1.19's epoll-fd becoming fd 2 / stderr only to be
>> +   replaced by /dev/null by OsInit() because the pollfd is not
>> +   writable, breaking ospoll_wait(). */
>> +open("/dev/null", O_WRONLY);

I'd suggest stripping out the OsInit code which messes with FDs now that
we leave stdin/stdout/stderr alone in the normal case. We should have
done this when the code messing with those FDs was removed.

> Hopefully no other threads in the X server are opening files, but if any ever
> do, it would be more reliable to do:
>
>   nullfd = open("/dev/null", O_WRONLY);
>   dup2(nullfd, 2);
>   close(nullfd);
>
> and let dup2 atomically close the old stderr and clone nullfd to it.

Yeah, this looks like a more reliable fix.

-- 
-keith


signature.asc
Description: PGP 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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[Alan Coopersmith]

On 01/ 9/17 07:57 AM, Hans de Goede wrote:

close(2);
+   /* Avoid xserver >= 1.19's epoll-fd becoming fd 2 / stderr only to be
+  replaced by /dev/null by OsInit() because the pollfd is not
+  writable, breaking ospoll_wait(). */
+   open("/dev/null", O_WRONLY);


Hopefully no other threads in the X server are opening files, but if any ever
do, it would be more reliable to do:

nullfd = open("/dev/null", O_WRONLY);
dup2(nullfd, 2);
close(nullfd);

and let dup2 atomically close the old stderr and clone nullfd to it.

--
-Alan Coopersmith-  alan.coopersm...@oracle.com
 Oracle Solaris Engineering - http://blogs.oracle.com/alanc
___
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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[Hans de Goede]

Hi,

On 09-01-17 10:00, Pierre Ossman wrote:

On 05/01/17 17:44, ipilc...@redhat.com wrote:

On Wednesday, October 5, 2016 at 4:13:48 AM UTC-5, Pierre Ossman wrote:

Alright. I'll have a look at doing the finishing touches. Thanks for the
patch. :)


FYI, there seems to be an issue with either the patch or v1.19 itself.

  https://bugzilla.redhat.com/show_bug.cgi?id=1408724



Urgh. I have no 1.19 system yet, so hopefully someone else can have a look.


Attached is a patch fixing this :)

Regards,

Hans
>From 372ff9d6754cd1b375836e5d4559061fb7be3496 Mon Sep 17 00:00:00 2001
From: Hans de Goede 
Date: Mon, 9 Jan 2017 16:03:30 +0100
Subject: [PATCH] Fix -inetd not working with xserver >= 1.19

xserver 1.19's OsInit will create a pollfd, followed by checking if fd 2 /
stderr is writable and if it is not, replacing fd 2 with /dev/null.

Since we close stderr in inetd mode to avoid xserver messages being send
to the client as vnc data, the pollfd becomes fd 2, only to be replaced
by /dev/null since a pollfd is not writable.

This commit fixes this by opening /dev/null directly after the close(2),
avoiding that the pollfd becomes fd 2.

Signed-off-by: Hans de Goede 
---
 unix/xserver/hw/vnc/xvnc.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/unix/xserver/hw/vnc/xvnc.c b/unix/xserver/hw/vnc/xvnc.c
index 2f3cd4a..a747654 100644
--- a/unix/xserver/hw/vnc/xvnc.c
+++ b/unix/xserver/hw/vnc/xvnc.c
@@ -575,6 +575,10 @@ ddxProcessArgument(int argc, char *argv[], int i)
 	dup2(0,3);
 	vncInetdSock = 3;
 	close(2);
+	/* Avoid xserver >= 1.19's epoll-fd becoming fd 2 / stderr only to be
+	   replaced by /dev/null by OsInit() because the pollfd is not
+	   writable, breaking ospoll_wait(). */
+	open("/dev/null", O_WRONLY);
 	
 	if (!displaySpecified) {
 	int port = vncGetSocketPort(vncInetdSock);
-- 
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

Re: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[Hans de Goede]

Hi,

On 09-01-17 10:00, Pierre Ossman wrote:

On 05/01/17 17:44, ipilc...@redhat.com wrote:

On Wednesday, October 5, 2016 at 4:13:48 AM UTC-5, Pierre Ossman wrote:

Alright. I'll have a look at doing the finishing touches. Thanks for the
patch. :)


FYI, there seems to be an issue with either the patch or v1.19 itself.

  https://bugzilla.redhat.com/show_bug.cgi?id=1408724



Urgh. I have no 1.19 system yet, so hopefully someone else can have a look.


I've been debugging this this afternoon, I believe I've identifief
the problem and I'm testing a fix now.

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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[Pierre Ossman]

On 05/01/17 17:44, ipilc...@redhat.com wrote:

On Wednesday, October 5, 2016 at 4:13:48 AM UTC-5, Pierre Ossman wrote:

Alright. I'll have a look at doing the finishing touches. Thanks for the
patch. :)


FYI, there seems to be an issue with either the patch or v1.19 itself.

  https://bugzilla.redhat.com/show_bug.cgi?id=1408724



Urgh. I have no 1.19 system yet, so hopefully someone else can have a look.

Regards
--
Pierre Ossman   Software Development
Cendio AB   https://cendio.com
Teknikringen 8  https://twitter.com/ThinLinc
583 30 Linköpinghttps://facebook.com/ThinLinc
Phone: +46-13-214600https://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
___
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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[ipilcher]

On Wednesday, October 5, 2016 at 4:13:48 AM UTC-5, Pierre Ossman wrote:
> Alright. I'll have a look at doing the finishing touches. Thanks for the 
> patch. :)

FYI, there seems to be an issue with either the patch or v1.19 itself.

  https://bugzilla.redhat.com/show_bug.cgi?id=1408724___
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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[Pierre Ossman]

On 04/10/16 17:45, Hans de Goede wrote:


If been working for 7 days on a row now to get 1.19 in
shape for Fedora 25, so I'm afraid that the v2 of this
patch I'm working on is going to be a take it or
leave it offer from my pov. You are of course more then
welcome to improve upon the patch yourself.



Alright. I'll have a look at doing the finishing touches. Thanks for the 
patch. :)




While on the subject of write-ready notification I noticed
what I believe is a bug in tigervnc-1.7.0/common/rdr/FdOutStream.cxx:
FdOutStream::flush(). When the stream is nonblocking and writeWithTimeout()
returns 0, then flush() will simply retry again (and again and again),
if I read the code correctly), so it will *busy* wait until all data
is written.



I'll have a look. It may simply be that flush() is defined as always 
blocking. I'll check who calls it.


Regards
--
Pierre Ossman   Software Development
Cendio AB   https://cendio.com
Teknikringen 8  https://twitter.com/ThinLinc
583 30 Linköpinghttps://facebook.com/ThinLinc
Phone: +46-13-214600https://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
___
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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[Hans de Goede]

Hi,

On 10/04/2016 04:45 PM, Pierre Ossman wrote:

On 03/10/16 17:59, Hans de Goede wrote:

Hello tigervnc devs,

As part of updating Fedora to xserver 1.19 I've written
a tiger vnc patch to make tigervnc work with xserver 1.19.



Nice, Thanks. :)


Since xserver 1.19 switches from select to poll the changes
are non trivial and require some #ifdef-s. The new code is
a lot cleaner then the old code though, which is good.

And with server 1.19 the os/WaitFor.c changes are no longer
necessary.



Very nice. However I'm not a fan of the large amount of duplicate code we're 
dragging around now. I'd prefer to switch everything over to the new model, and 
put something in vncBlockHandler.c that emulates the new API on top of the old 
system.


If been working for 7 days on a row now to get 1.19 in
shape for Fedora 25, so I'm afraid that the v2 of this
patch I'm working on is going to be a take it or
leave it offer from my pov. You are of course more then
welcome to improve upon the patch yourself.


Attached is a tigervnc-1.7.0 patch, as well as a patch
to apply to the xserver sources to patch in the tigervnc
ext and hw/vnc dir into the buidlsys.




+#include "os.h"


NAK on this I'm afraid. Using Xorg headers in C++ has been endless grief. Hence 
the wrappers in both directions. I'd say put this in vncBlockHandler.c and tie 
it up via vncExtInit.cc.


See above, I'm completely out of steam for any more 1.19
related work, sorry. feel free to fix this up before
merging.


+  SetNotifyFd(sock->getFd(), HandleSocketFd, X_NOTIFY_READ, this);


I don't see any code setting X_NOTIFY_WRITE?


Yes, while working on something completely unrelated I realized that
I might have deleted to much code from the BlockHandler, the code
that used to deal with setting X_NOTIFY_WRITE in the BlockHandler
was also dealing with (*i)->isShutdown(), then I realized that
(*i)->isShutdown() could / should be checked at the end of the
FdNotify callback, and when removing that from the BlockHandler,
I accidentally also deleted the setting of X_NOTIFY_WRITE.

I was preparing a v2 of the patch which re-introduces this,
when I noticed this mail (while waiting for the updated patch
to compile :)

I will post a v2 soon.

Regards,

Hans


p.s.

While on the subject of write-ready notification I noticed
what I believe is a bug in tigervnc-1.7.0/common/rdr/FdOutStream.cxx:
FdOutStream::flush(). When the stream is nonblocking and writeWithTimeout()
returns 0, then flush() will simply retry again (and again and again),
if I read the code correctly), so it will *busy* wait until all data
is written.




--- xserver/include/os.h~2016-10-03 09:07:29.0 +0200
+++ xserver/include/os.h2016-10-03 14:13:00.013654506 +0200
@@ -621,7 +621,7 @@
 extern _X_EXPORT void
 LogClose(enum ExitCode error);
 extern _X_EXPORT Bool
-LogSetParameter(LogParameter param, int value);
+LogSetParameter(enum _LogParameter param, int value);
 extern _X_EXPORT void
 LogVWrite(int verb, const char *f, va_list args)
 _X_ATTRIBUTE_PRINTF(2, 0);


Is this a fix that's meant to go upstream at some point?

Regards

___
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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc

[Pierre Ossman]

On 03/10/16 17:59, Hans de Goede wrote:

Hello tigervnc devs,

As part of updating Fedora to xserver 1.19 I've written
a tiger vnc patch to make tigervnc work with xserver 1.19.



Nice, Thanks. :)


Since xserver 1.19 switches from select to poll the changes
are non trivial and require some #ifdef-s. The new code is
a lot cleaner then the old code though, which is good.

And with server 1.19 the os/WaitFor.c changes are no longer
necessary.



Very nice. However I'm not a fan of the large amount of duplicate code 
we're dragging around now. I'd prefer to switch everything over to the 
new model, and put something in vncBlockHandler.c that emulates the new 
API on top of the old system.



Attached is a tigervnc-1.7.0 patch, as well as a patch
to apply to the xserver sources to patch in the tigervnc
ext and hw/vnc dir into the buidlsys.




+#include "os.h"


NAK on this I'm afraid. Using Xorg headers in C++ has been endless 
grief. Hence the wrappers in both directions. I'd say put this in 
vncBlockHandler.c and tie it up via vncExtInit.cc.



+  SetNotifyFd(sock->getFd(), HandleSocketFd, X_NOTIFY_READ, this);


I don't see any code setting X_NOTIFY_WRITE?


--- xserver/include/os.h~   2016-10-03 09:07:29.0 +0200
+++ xserver/include/os.h2016-10-03 14:13:00.013654506 +0200
@@ -621,7 +621,7 @@
 extern _X_EXPORT void
 LogClose(enum ExitCode error);
 extern _X_EXPORT Bool
-LogSetParameter(LogParameter param, int value);
+LogSetParameter(enum _LogParameter param, int value);
 extern _X_EXPORT void
 LogVWrite(int verb, const char *f, va_list args)
 _X_ATTRIBUTE_PRINTF(2, 0);


Is this a fix that's meant to go upstream at some point?

Regards
--
Pierre Ossman   Software Development
Cendio AB   https://cendio.com
Teknikringen 8  https://twitter.com/ThinLinc
583 30 Linköpinghttps://facebook.com/ThinLinc
Phone: +46-13-214600https://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
___
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