[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.000000000 +0200
>> +++ xserver/include/os.h    2016-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