http://bugs.winehq.org/show_bug.cgi?id=17195
Summary: NamedPipe datagrams need to be _really_ datagrams Product: Wine Version: unspecified Platform: Other OS/Version: other Status: UNCONFIRMED Severity: normal Priority: P2 Component: ntdll AssignedTo: wine-bugs@winehq.org ReportedBy: lkcl@lkcl.net
ok, after a little bit of investigation, i think i understand the pipes code enough to be able to say what's going on, and i'm seeing something like this:
process 1:
recvpipe = CreateNamedPipe("\pipe\fred") ReadFile(recvpipe, buffer, &length); printf(length) ===> 43 ReadPipe(recvpipe, buffer, &length); /* no data */
process 2:
sendpipe = CreateNamedPipe("\pipe\fred") length = 9; WriteFile(sendpipe, buffer, &length); length = 34; WriteFile(sendpipe, buffer, &length);
what's happening is that the data being sent down the pipes isn't being done as datagrams. the implementation is using a stream-based fd.
the solution is: you _must_ implement a protocol on top of the pipes which sends the length (as a 32-bit int, whatever) which is read off the fd, followed by the data stream of _exactly_ that length.
_must_. there's no two ways about this.
the protocol of Pipes is unfortunately a combination of both datagrams and streams. datagrams because the lengths of data sent are absolute and inviolate. streams because the data order and reliability are _also_ absolute and inviolate.
you can't use datagrams (because they're unreliable). you can't expect all unixen to support datagrams on top of unix sockets (if that's what's being used).
so - you have to send the length, as part of the implementation of the pipe-data-send.
sending a length will also solve the issue of trying to send zero-length pipe datagrams.
as a first implementation, you _might_ be able to get away with assuming that when someone asks for some data, they _will_ provide a buffer big enough.
... actually... i don't see any ERR_MORE_DATA error codes in NtReadFile, so that would explain... this is going to get messy :)
http://bugs.winehq.org/show_bug.cgi?id=17195
Juan Lang juan_lang@yahoo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW Ever Confirmed|0 |1
--- Comment #1 from Juan Lang juan_lang@yahoo.com 2009-01-30 12:32:43 --- This is a longstanding bug in our named pipe implementation. We never got around to fixing it because a) it's harder than not doing so, and b) we hadn't yet found an app that needed it.
Do the python tests depend on this being correct?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #2 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-01-30 14:05:10 --- (In reply to comment #1)
This is a longstanding bug in our named pipe implementation. We never got around to fixing it because a) it's harder than not doing so, and b) we hadn't yet found an app that needed it.
well, there are two. one exists - python 2.7 - the new multithreading module uses named pipes in message mode - and messages are used to exchange a digest-based authentication across processes, as a cheap way of not checking the security context. the exchange is interfered with by the first _actual_ bit of data that needs to be sent being "glommed" onto the end of the last part of the authentication exchange.
so instead of 9 bytes being read "#SUCCESS#", 43 bytes are read - "#SUCCESS#firstpacket....." and thus it all goes pear-shaped.
the second one app - that doesn't yet exist - is for named pipe integration with samba. Named Pipes over SMB allow MessageMode to be set. the DCE/RPC layer (ncacn_np) _definitely_ sets MessageMode.
i saw well over 8mb of data being exchanged over one "message", at hp. they had well over 2,000 printers on one domain. it took _forever_ to complete the transfer (order n-squared) because some idiot in microsot implemented "ERR_MORE_DATA" over an MSRPC interface in the spoolss service. so it first asked for 64k, then 128k, then 192k .... all the way up to 8mb!!!
all using NamedPipes. yeuuch :)
Do the python tests depend on this being correct?
oh yeah. the entire multiprocessing infrastructure now depends critically on this working correctly. they're moving away from the posix implementation of popen(), and also moving away from all sorts of other bunged-up stuff. all in the new module "multiprocessing" which abstracts out pipe communication, forking, threading, messaging, inter-process communication, all this lovely stuff.
it's already become a major part of the python infrastructure for 2.7 and 3.0, so it's a bit of a big deal that this works.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #3 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-01-31 05:19:15 --- Created an attachment (id=19121) --> (http://bugs.winehq.org/attachment.cgi?id=19121) start of test app for messagemode
ok, you start this with: ./msrpcd.exe or if you want a single run: ./msrpcd.exe -p pipename
and then, client-side: ./msrpc.exe -p pipename -t testname
where the two tests so far are test1 and send2recv2
test1 is the simple send 1 recv 1 send 1 recv 1
send2recv2 as its name suggests sends 2 messages and then waits to receive 2.
but, due to the bug in messagemode, it only receives one, of total size of the glomming of both messages.
debug output. exit code is 1 on failure.
msrpcd.exe will run continuously, default pipe name "test", if you get bored of re-running it all the time.
internally, msrpcd.exe will create a new process, and back-to-back proxy the data between the "front" msrpcd.exe and the "child" msrpcd.exe over a unique pipe named after the thread-id.
i didn't cut this code out because i thought it would be useful, later.
it's not particularly well-written - there are better ways (such as use of DupHandle and other stuff) but hey, it was my first experiment with NamedPipes and i'm proud of it :)
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19121|0 |1 is obsolete| |
--- Comment #4 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-01-31 09:15:29 --- (From update of attachment 19121) extra test (short read)
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #5 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-01-31 09:19:59 --- Created an attachment (id=19124) --> (http://bugs.winehq.org/attachment.cgi?id=19124) added short read test
ok, this extra test is called shortread: ./msrpcd.exe & ./msrpc.exe -t shortread
it demonstrates that "short read" is actually very straightforward to implement. both ReadFile and PeekNamedPipe return the number of bytes requested - _if_ any are requested, and ERROR_MORE_DATA is returned if there are bytes remaining. the difference between ReadFile and PeekNamedPipe is that two extra params are tacked on the end: available and remaining. both are pointers and the sums should be:
read_this_time + remaining == available.
i need to do another test which does a short read using PeekNamedPipe followed by a ReadFile, to confirm that absolutely.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #6 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-01-31 15:21:33 --- Created an attachment (id=19132) --> (http://bugs.winehq.org/attachment.cgi?id=19132) beginnings of messagemode
this isn't ready for committing but i'm looking for advice on how best to proceed - i've duplicated a function in two places (kernel32 and ntdll) because i don't know where to put it; the set_named_pipe_info server function is not receiving the "availdata" info - or if it is, it's getting overwritten and i don't exactly know how.
but - this _does_ pass the send2recv2 test!
just not the shortread one which does PeekNamedPipe(), that's where it goes a bit wrong.
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19132|0 |1 is obsolete| |
--- Comment #7 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-01 04:27:47 --- Created an attachment (id=19145) --> (http://bugs.winehq.org/attachment.cgi?id=19145) passes all 3 tests
ok, the last thing that i missed, as of last night, was filling in the message size field in PeekNamedPipe - that was all :) so, now this patch passes all three tests!
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19124|0 |1 is obsolete| |
--- Comment #8 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-02 13:25:27 --- Created an attachment (id=19215) --> (http://bugs.winehq.org/attachment.cgi?id=19215) 4th test - threadread
this is running a writer-thread and a reader-thread; 50 messages are sent, of 512 bytes each. short reads of 16 bytes are used to retrieve each message. this test generally making things keel over quite badly :) xp also gets pretty unhappy if you increase the number of messages to 500. there's a limit you can set, somewhere, i believe. haven't found it yet.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #9 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-04 09:39:21 --- fascinating.
a test i'm writing - threadwrites - has the main process doing blocking-reads, and there are 5 threads doing writes. the test _hangs_ on NT.
writes in threads can never be performed at the same time as there's a blocking read outstanding.
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19145|0 |1 is obsolete| | Attachment #19215|0 |1 is obsolete| |
--- Comment #10 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-04 12:14:34 --- Created an attachment (id=19240) --> (http://bugs.winehq.org/attachment.cgi?id=19240) 5th test - threadwrite
this is a multi-threaded-writer test. several threads can be made to write on the same pipe handle, and there is only one reader. the number of messages sent and received are counted, and the data sent is checked to be correctly received.
it's using CreateThread() not _beginthread() due to a bug in nt with the msvcrt function, _beginthread(), which blocks indefinitely on multi-threaded read/write interaction!
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends on| |17263
http://bugs.winehq.org/show_bug.cgi?id=17195
Juan Lang juan_lang@yahoo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |juan_lang@yahoo.com
--- Comment #11 from Juan Lang juan_lang@yahoo.com 2009-02-05 15:12:51 --- I tried building and running this here. I modified Makefile to use winegcc rather than '/usr/local/bin/wine gcc.exe'. I then tried:
$ ./msrpcd -p mypipe
That produced this output: open_server_pipe: \.\pipe\mypipe open_server_pipe succeeded
I then ran: $ ./msrpc -p mypipe -t test1
The server produced this output: Named Pipe connected msrpcd_process: connection received not ok 0x1c error 6d not ok 0x1c error 6d
The client produced this output: ncalrpc_l_establish_connection: connecting to .[mypipe] Opening pipe \.\pipe\mypipe SetNamedPipeHandleState failedncalrpc_l_establish_connection: failed mypipe) connection failed msrpc_send: data: 0x7c703698 len 17 sent data failed
Is this what you expect would happen under Wine? What would happen under Windows?
Updated instructions, expected results, etc. would help. Thanks!
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #12 from Juan Lang juan_lang@yahoo.com 2009-02-05 15:14:09 --- Oh, I forgot to say, I also modified msrpc.c. I removed the #include <process.h>, and changed the _beginthread to CreateThread, and changed the _sleep to Sleep. If that's a problem let me know ;-)
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #13 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-05 15:30:16 --- hiya juan,
msrpc.exe exits with a code of 1 in all instances of failure, and 0 on success, so that it can be run from automated shell scripts.
the _very_ first clue is this:
"SetNamedPipeHandleState failed"
if you look at the current implementation of SetNamedPipeHandleState, it's this:
BOOL WINAPI SetNamedPipeHandleState( HANDLE hNamedPipe, LPDWORD lpMode, LPDWORD lpMaxCollectionCount, LPDWORD lpCollectDataTimeout) { return FALSE; /* FIXME */ }
so at almost the _very_ first hurdle, the current code in wine is wrong :)
the return result from this function must be TRUE not FALSE.
if you modify just that one line, then you will get wine to at least pass "test1". from there-on, all the other tests are guaranteed to fail.
http://bugs.winehq.org/show_bug.cgi?id=17195
Juan Lang juan_lang@yahoo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends on| |17273
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #14 from Juan Lang juan_lang@yahoo.com 2009-02-05 16:05:44 --- (In reply to comment #13)
"SetNamedPipeHandleState failed"
Roger that, I created 17273 to track that error, and I'll try to write up some Alexandre-acceptable test cases for that function.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #15 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-06 04:02:26 --- well, this is the implementation i have so far:
/*********************************************************************** * SetNamedPipeHandleState (KERNEL32.@) */ BOOL WINAPI SetNamedPipeHandleState( HANDLE hNamedPipe, LPDWORD lpMode, LPDWORD lpMaxCollectionCount, LPDWORD lpCollectDataTimeout) { /* should be a fixme, but this function is called a lot by the RPC * runtime, and it slows down InstallShield a fair bit. */ TRACE("%p %p/%d %p %p\n", hNamedPipe, lpMode, lpMode ? *lpMode : 0, lpMaxCollectionCount, lpCollectDataTimeout);
if (lpMode) { /* FIXME: This function should fail if PIPE_READMODE_MESSAGE * is specified for a byte-type pipe. * TODO: write a test that deliberately tries this on nt, * to find out the error code. */ int flgs = 0; if ((*lpMode) & PIPE_READMODE_MESSAGE) { /* FIXME: the current "are we in message-mode" is based * on NAMED_PIPE_MESSAGE_STREAM_WRITE being set, but * this is READMODE being asked to be set - why? */ flgs |= NAMED_PIPE_MESSAGE_STREAM_READ; } if ((*lpMode) & PIPE_NOWAIT) { flgs |= NAMED_PIPE_NONBLOCKING_MODE; } server_set_named_pipe_info( hNamedPipe, NAMED_PIPE_INFO_SET_FLAGS, flgs, 0 ); return TRUE; } return TRUE; /* FIXME: ignore all but lpMode setting, for now */ }
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #16 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-07 08:39:25 --- tests TODO:
1) thread multiwrite, thread multiread.
this test will show whether it's possible to have multiple concurrent readers and writers at the _same_ time. i.e.:
wth-1 : write starts (e.g. 2mb of data) wth-2 : write starts (e.g. 2mb of data) rth-1 : read starts whilst wth-1 and wth-2 are still writing. rth-2 : read starts of SECOND message whilst wth-1 and wth-2 are still writing, and rth-1 is still reading message number 1.
2) start off in non-message-mode and switch to message-mode with data still unread
this test will be interesting. basically, the SetNamedPipeHandleState is to be delayed until a few messages have been exchanged, and some data left in the pipe. a special focus will be on determining whether the data already in the pipe is:
a) thrown away b) "turned into" a message-mode message c) prepended to the next message that's written.
3) start off in message-mode and switch to non-message-mode with data outstanding and not outstanding, as two separate tests.
4) do a CreateNamedPipe in message-mode on server-end and don't set message-mode on the client, and see what happens
5) do a CreateNamedPipe _not_ in message-mode on server-end, and set message-mode on the server, and see what happens.
6) more - yet to be thought of.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #17 from Juan Lang juan_lang@yahoo.com 2009-02-07 19:23:53 --- (In reply to comment #16)
- start off in non-message-mode and switch to message-mode with data still
unread
I believe this isn't allowed. See the existing tests, dlls/kernel32/tests/pipe.c, lines 213-214: /* trying to change the client end of a byte pipe to message mode should fail */ ok(!SetNamedPipeHandleState(hFile, &lpmode, NULL, NULL), "Change mode\n");
On the other hand, it's possible that these tests only test the client. Perhaps the server can change modes? I suspect not, but you're right that a test is needed.
- start off in message-mode and switch to non-message-mode with data
outstanding and not outstanding, as two separate tests.
I believe this is possible, at least for the client, in contrast to test 2). I don't believe there are tests for this though.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #18 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-08 09:57:49 --- Created an attachment (id=19335) --> (http://bugs.winehq.org/attachment.cgi?id=19335) progress so far
work-in-progress, provided in case anyone's interested. being absolutely honest, it's fairly horrendous! :)
the infrastructure is in place to create, on each messagemode write, another socketpair, but is disabled for now (line 1804 of server/named_pipe.c) as the socketpair needs to be closed when availdata == 0 (line 1665, TBD) on a read, and in ntdll/file.c detection is required, if polling / waiting for a read, for when a close occurs, to loop round and get another filedescriptor.
so that's _definitely_ one for another day!
right now, i have at least test1 working, and, amazingly, shortread.
send2recv2 obviously fails due to the use of a single filedescriptor pair.
ironically, testing is actually taking place by having to watch the \pipe\svcctl rpcrt4 interaction, not the tests! named pipes are such a fundamental part of wine now that ... ok, it would be _really_ helpful if ncalrpc used lrpc not named pipes :)
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends on| |17316
http://bugs.winehq.org/show_bug.cgi?id=17195
Bug 17195 depends on bug 17263, which changed state.
Bug 17263 Summary: missing expectation of ERROR_MORE_DATA status code in rpcrt4_conn_np_read http://bugs.winehq.org/show_bug.cgi?id=17263
What |Old Value |New Value ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution| |FIXED
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #19 from Juan Lang juan_lang@yahoo.com 2009-02-11 12:19:08 --- (In reply to comment #16)
- start off in non-message-mode and switch to message-mode with data still
unread
As I expected, this isn't allowed, either on the client end or server end of a named pipe. My tests that demonstrate it were sent in: http://www.winehq.org/pipermail/wine-patches/2009-February/069264.html
- start off in message-mode and switch to non-message-mode with data
outstanding and not outstanding, as two separate tests.
This, in contrast, is possible. I haven't tested reading and writing in this case, just that the API to switch modes succeeds. See the above link.
- do a CreateNamedPipe in message-mode on server-end and don't set
message-mode on the client, and see what happens
Also possible.
- do a CreateNamedPipe _not_ in message-mode on server-end, and set
message-mode on the server, and see what happens.
Not possible. See tests.
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19335|0 |1 is obsolete| |
--- Comment #20 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-12 04:59:15 --- Created an attachment (id=19405) --> (http://bugs.winehq.org/attachment.cgi?id=19405) passes test1
the monumental historic and epic battle to do correct named pipes semantics wins a small victory: test1 is passed. sadly that's the _only_ test passed so far but it's a start :) i can't quite get over how horrendous the design is.
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19405|0 |1 is obsolete| |
--- Comment #21 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-14 11:39:20 --- Created an attachment (id=19446) --> (http://bugs.winehq.org/attachment.cgi?id=19446) nearly there!
amazing! it's nearly there! there is some sort of memory corruption going on after a series of short reads (4 or more results in memory corruption). if you shorten the number of reads, all five tests are passed! it's almost there.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #22 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-14 11:41:05 --- to get a full quotes pass quotes, change line 197 #define MSG_SIZE 32 in msrpc.c test suite, and everything will work. as soon as you up that to 64 or 128, splat. no idea why, yet.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #23 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-14 12:04:16 --- Created an attachment (id=19449) --> (http://bugs.winehq.org/attachment.cgi?id=19449) design / data structure document
here's a write-up of the design i had to do. socketpairs have to be used in "simplex" NOT duplex. if you try to use a socketpair for both reading and writing.... err... how? you have ten messages written by client-to-server but only two written server-to-client, how do you preserve message-mode semantics _and_ save a few socketpairs? i don't want to think about how nasty the design would be - safer to just use the socketpairs for a single purpose: write in only one of them, and only read from the other.
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19449|application/x-pdf |application/pdf mime type| |
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19446|0 |1 is obsolete| |
--- Comment #24 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-15 09:21:09 --- Created an attachment (id=19474) --> (http://bugs.winehq.org/attachment.cgi?id=19474) messagemode tests passed if BUF_SIZE reduced
this version successfully passes the tests _if_ you don't ask it to hand out 200 filedescriptors. also it looks like non-message-mode has been disrupted, so will write some tests for that.
but - it works!
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Kenneth Casson Leighton lkcl@lkcl.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19474|0 |1 is obsolete| |
--- Comment #25 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2009-02-15 12:10:46 --- Created an attachment (id=19477) --> (http://bugs.winehq.org/attachment.cgi?id=19477) messagemode tests passed if BUF_SIZE reduced
other than the insane numbers of file descriptors, and the excessive amount of debug output, i'm happy with this.
http://bugs.winehq.org/show_bug.cgi?id=17195
Juan Lang juan_lang@yahoo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19477|application/octet-stream |text/plain mime type| |
http://bugs.winehq.org/show_bug.cgi?id=17195
Bug 17195 depends on bug 17316, which changed state.
Bug 17316 Summary: ncalrpc is using named pipes not lrpc http://bugs.winehq.org/show_bug.cgi?id=17316
What |Old Value |New Value ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution| |WONTFIX
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #26 from Austin English austinenglish@gmail.com 2009-08-26 13:20:57 --- Is this still present in current (1.1.28 or newer) wine?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #27 from Juan Lang juan_lang@yahoo.com 2009-08-26 15:23:45 --- Yep, no change.
http://bugs.winehq.org/show_bug.cgi?id=17195
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |download, patch, testcase
--- Comment #28 from Austin English austinenglish@gmail.com 2010-09-07 14:42:59 CDT --- Still present.
http://bugs.winehq.org/show_bug.cgi?id=17195
Dan Kegel dank@kegel.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |adys.wh@gmail.com
--- Comment #29 from Dan Kegel dank@kegel.com 2011-03-22 16:54:38 CDT --- *** Bug 26018 has been marked as a duplicate of this bug. ***
http://bugs.winehq.org/show_bug.cgi?id=17195
Dan Kegel dank@kegel.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dank@kegel.com
--- Comment #30 from Dan Kegel dank@kegel.com 2011-03-22 16:57:31 CDT --- Ubisoft Launcher (Assassin's Creed brotherhood, etc.) is also said to use named pipes. Dunno if it uses message mode though.
http://bugs.winehq.org/show_bug.cgi?id=17195
Berillions berillions@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |berillions@gmail.com
--- Comment #31 from Berillions berillions@gmail.com 2011-03-23 02:11:37 CDT --- The latest patch create by Luke Kenneth Casson Leighton doesn't work with the latest version of wine (1.3.16) : http://bugs.winehq.org/attachment.cgi?id=19477
I don't know how to do to modify the patch to resolv the problem...
http://bugs.winehq.org/show_bug.cgi?id=17195
James McKenzie jjmckenzie51@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jjmckenzie51@gmail.com
--- Comment #32 from James McKenzie jjmckenzie51@gmail.com 2011-03-27 18:34:38 CDT --- @Luke: Is it ok for someone else to pick up your code and update it to the latest Wine development version? @Juan: What was the latest on this? Was anything for this accepted by AJ?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #33 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-03-27 18:53:59 CDT --- it's free software, you don't need my permission :)
you do, however, need to take into account the fact that this is a severely resource-hungry non-optimised solution which can in no way be put into a production environment.
to properly solve this issue, i'm afraid that wine is going to need a quite significant architectural "upgrade": wineserver must become multi-threaded.
this would also solve other quite serious and subtle bugs/issues including a long-standing performance issue (in certain circumstances) across a broad range of win32 applications.
http://bugs.winehq.org/show_bug.cgi?id=17195
Dmitry Timoshkov dmitry@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Component|ntdll |-unknown
--- Comment #34 from Dmitry Timoshkov dmitry@codeweavers.com 2011-03-28 00:51:00 CDT --- (In reply to comment #33)
this would also solve other quite serious and subtle bugs/issues including a long-standing performance issue (in certain circumstances) across a broad range of win32 applications.
Sounds like a pure speculation.
http://bugs.winehq.org/show_bug.cgi?id=17195
Maarten Lankhorst m.b.lankhorst@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |m.b.lankhorst@gmail.com
--- Comment #35 from Maarten Lankhorst m.b.lankhorst@gmail.com 2011-05-19 06:09:29 CDT --- just curious, can you update it to current wine? Also it seems your patch forgot to modify server/protocol.def which should be used to update other related files with tools/make_requests
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #36 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-05-19 08:24:11 CDT --- (In reply to comment #35)
just curious, can you update it to current wine? Also it seems your patch forgot to modify server/protocol.def which should be used to update other related files with tools/make_requests
apologies, maarten - i can only do this if i am paid for my time to do so. with £25,000 of debt to repay, which is scheduled for repayment somewhere in 2036 (assuming that interest is zero), as well as having received income last year of approximately £8,000 when i required £13,000 to cover all costs, you can appreciate that doing "freebie" work, especially with having to also spend time to deal with public and permanently-archived comments such as "you are being incredibly lazy" and "that sounds like pure speculation", would be unwise.
l.
http://bugs.winehq.org/show_bug.cgi?id=17195
Alex Bradbury asb@asbradbury.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |asb@asbradbury.org
http://bugs.winehq.org/show_bug.cgi?id=17195
LBM knaprigt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |knaprigt@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
Valentyn Pavliuchenko valentyn.pavliuchenko@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |valentyn.pavliuchenko@gmail | |.com
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #37 from Adam Martinson adam.r.martinson@gmail.com 2011-12-14 16:57:46 CST --- Created attachment 37977 --> http://bugs.winehq.org/attachment.cgi?id=37977 patch (work in progress)
Requires patches from bug 17273. This approach may not work for named pipes accessed over a network, I'd have to look into that more.
Any questions/advice/etc welcome.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #38 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-12-14 17:40:51 CST --- adam, hi,
it's good to see someone finally working on this. here's some notes on what i discovered.
* the use of one socket per message, which was the only way to get the correct kind of blocking semantics, very very quickly exhausts the max number of open filedescriptors of any system.
* logically, therefore, the only way in which the correct blocking semantics (on a filedescriptor) can be achieved is also not a viable way.
* therefore, logically, there is no point in pursuing - by enhancement or otherwise - the approach taken in this demonstration patch which otherwise successfully and correctly implements proper namedpipe datagrams.
* therefore, logically, this has to be done correctly.
the correct way to do this is for the blocking semantics to be implemented as a *userspace* service (running under wine). it's to do with threads, and the fact that the wine "server" is single-threaded, blocking, and is designed around the "asynchronous" concept.
the alternative is a complete nightmare, requiring *everything* to do with NamedPipes (networked or otherwise) to be *entirely* implemented as "kernel-side" - i.e. asynchronous and single-threaded.
that includes services as well.
now, naively, that sounds all lovely and wonderful, until you go looking at the asynchronous libsmb code in samba, which is of course GPL'd so it *cannot* be used and would require complete reimplementation, and you find that it's tens of thousands of lines of code.
there is a much simpler alternative: use *existing* synchronous libraries, code and services - in fact you could simply modify samba (or better have a look at samba-tng which already has been ported to Win32) and *don't* go reinventing the wheel.
howeverrrr... before you get there, you have to provide a means by which client userspace applications can call into kernelspace, and for kernelspace to call *back* to userspace [the alternative being to reimplement all of said libraries and services as asynchronous code, in kernelspace].
if you've been following closely up until now, you may have an inkling of what the problem is.
the problem is that when a wine userspace application calls a kernelspace function call (e.g. WriteFile on a pipe), there's only one "thread", kernel-side, and it permanently and completely blocks *all* and *any* userspace applications until that quotes kernelspace quotes function has been serviced.
in other words, if you passed the data on to another userspace application, it would cause deadlock.
the solution is: to make wineserver multi-threaded.
this is, btw, unavoidable, absolutely essential, and it is inevitable that wineserver _will_ have to be made multi-threaded. denial of this fact will only continue to result in obscure and inexplicable failures over the years, inexplicable slow operation of wine under certain conditions, and so on.
now, the last time i described this, i was told, incredibly curtly and very rudely, that "this was pure speculation" by dmitri (comment #34)
if i get that kind of shit treatment again, which is entirely unacceptable in a free software environment, i will simply request that my account on bugs.winehq.org be deleted, and you (plural) will never receive the benefit of my advice or experience in this highly complex and specialist area, ever again.
anyway.
you'll notice in NT documentation that several function calls are marked as kernel-level blocking or otherwise. it really is absolutely critical for wineserver to mirror this behaviour. it may be worthwhile seeing what the ReactOS team have done.
also, given that wine is itself so fundamentally, fundamentally dependent on NamedPipes, now, it really really is essential to implement a lrpc namespace in ncalrpc, so that the fundamental services such as \pipe\svcctl can operate unimpeded whilst development and testing is taking place! ncalrpc using the NamedPipe functions is *incorrect*. the implementation of ncalrpc doesn't actually matter - it could be a complete duplicate of the entire NamedPipe code right now just with function call renames as long as it's fixed later (to also have correct message-passing semantics).
so... lots to do, but this is all pretty low-level and critical stuff, it's a credit to the implementation of wine that it's got this far _without_ tackling these thorny issues :)
l.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #39 from Dan Kegel dank@kegel.com 2011-12-16 09:11:38 CST --- Ubuntu has quadrupled the default limit on file descriptors since you last checked, so running out of them might not be a problem now.
If you think we should ask them to double it again, now's a good time, while 11.10 is still alpha.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #40 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-12-16 12:16:02 CST --- dan, apologies for not making this clearer: potentially hundreds of thousands of file handles will be required. this patch is NOT production-suitable, you really do have to bite the bullet and fix wineserver properly.
this patch is for "demonstration purposes" ONLY and it would be a severe bug if it were included, as it could be used to attack a system through resource starvation simply by opening hundreds or thousands of NamedPipes and sending hundreds or thousands of messages on each pipe.
sorry, dan - this is about limitations in the design of wine that simply haven't had to be tackled up until now: it's now time to deal with them if wine is to ever progress to the next level of interoperability.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #41 from Dan Kegel dank@kegel.com 2011-12-16 12:25:36 CST --- Leaving aside integration with Samba, which won't happen for a while, I don't see that an interim implementation of named pipes that need two fd's per pipe would be any more vulnerable than the current situation; you can already cause fd starvation if you realy want to.
Something that get pass the python tests and made Asassin's Creed Brotherhood work would be very nice, even if it wasn't sufficient for all purposes.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #42 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-12-16 14:16:01 CST --- dan, it's more than that: you're forgetting that NamedPipes are fundamental and critical to wine at a low-level. i've mentioned several times now that the implementation of ncalrpc [incorrectly] uses NamedPipes when it should be using an implementation of local-communication [that *can* be kernel-based or wine's equivalent thereof].
even just doing service start, stop and other management; registry edits and reads, *everything* that's fundamental to nt is based around MSRPC, and that means that everything that's fundamental is critically dependent [accidentally] on NamedPipes.
you will cause *massive* resource starvation. do a profile (gprof) on a range of applications (or use debug grepping, whatever) looking for WriteFile and ReadFile and Transact2 on NamedPipes. i think you'll find that even running "ordinary" applications has a severe performance penalty as well as massive resource usage.
i wrote this patch as a demo, and proved that yes it can be done but it is *not* suitable for production usage.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |adam.r.martinson@gmail.com
--- Comment #43 from Adam Martinson adam.r.martinson@gmail.com 2011-12-17 17:41:11 CST --- (In reply to comment #38) Don't take what Dmitry says so personally, it may sound like he's trolling but that's not his intention. I'm tempted to agree with you on multithreading, but if that is indeed something that needs to be done Alexandre probably either has to do it himself, or at least be heavily involved in the process.
As I understand it this bug has 2 parts: 1 - FILE_PIPE_MESSAGE_MODE/PIPE_READMODE_MESSAGE: Reads from the pipe get at most 1 message. If the provided buffer is insufficient the remainder of the message is left in the pipe and STATUS_MORE_ENTRIES/ERROR_MORE_DATA results. Using SOCK_SEQPACKET instead of SOCK_STREAM isn't an option, because reads on a SOCK_SEQPACKET socket cause any message remainder to be discarded in the case of an insufficient buffer.
The solutions I've considered for this are either implementing datagrams on top of SOCK_STREAM sockets, as in my patch, or creating a pair of SOCK_SEQPACKET sockets for writing and a pair of SOCK_STREAM sockets for reading, with a server thread that waits for a message on the SOCK_SEQPACKET socket to be available and the SOCK_STREAM socket to be empty, and then relays a single message. The latter is much more of a headache to implement which is why I went with the first option.
2 - FILE_PIPE_COMPLETE_OPERATION/PIPE_NOWAIT: Can you elaborate more on this part as far as current behavior vs correct behavior? I haven't started tackling this part. I'm not convinced that correct behavior here is incompatible with my patch, but I need to understand the problem better.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37977|0 |1 is obsolete| |
--- Comment #44 from Adam Martinson adam.r.martinson@gmail.com 2011-12-19 03:59:25 CST --- Created attachment 38025 --> http://bugs.winehq.org/attachment.cgi?id=38025 patch 1/2 (work in progress)
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #45 from Adam Martinson adam.r.martinson@gmail.com 2011-12-19 04:02:00 CST --- Created attachment 38026 --> http://bugs.winehq.org/attachment.cgi?id=38026 patch 2/2 (work in progress)
This *should* make FILE_PIPE_COMPLETE_OPERATION/PIPE_NOWAIT work, for NtReadFile() anyhow... Luke, is that what you mean by blocking semantics?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #46 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-12-19 07:11:26 CST --- sorry adam it's been a while, but if that's a value of 0x3 then yes it's quite likely to be the case.
the order in which the work really needs to be done is:
* create ncalrpc which is identical functionality to existing *broken* NamedPipes * prove that test cases work by using broken and completely unacceptable method of use of socket pairs, use of which would otherwise result in wine being classed as a security violation [resource starvation] * decide what to do from here:
path 1)
* decide that it's perfectly acceptable to create complete, total, utter reimplementations of all services when the time comes to tackle these things - all services which *could* be "farmed out" into userspace are instead forcibly implemented as wine-equivalent to kernelspace i.e. asynchronous. this will be hundreds of thousands of lines of code.
path 2)
* add kernel-level multithreading to wineserver. it involves insertion of global locks around data structures that have, until now, not been necessary. basically the entire wineserver codebase needs to be properly made multithreaded.
* then write that proper "shim" which allows redirection of kernelspace pipe data out to userspace, allowing individual kernel-level threads to "block" until the *userspace* program feeds back the response.
i don't want to discourage you from working on this adam but this really is a "bite the bullet and get on with it" point for the wine team.
of course, i am not a wine team member, nor have i yet received the offers of payment which would allow me to help do the required work, so i have to leave it to the wine team members to decide whether it is more useful to them to continue with the situation as-is or whether it is more useful for them to evaluate and take the advice above.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #47 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-12-19 07:14:54 CST --- bottom line, adam, i apologise but until wine has proper multithreading there's no... well i won't call it incentive... how can i put this... if i help you as things stand, it is just to create "yet another test system which cannot, ever, be deployed live". there already exists such a system, it's the patches that i originally created.
you can run the tests... actually it's best to do the ncalrpc work first... because if you make *one* tiny error then it affects even the _startup_ code of wine (svcctl).
that would be the most useful thing for you to do right now - implement an ncalrpc that is independent of NamedPipes.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #48 from Alexandre Julliard julliard@winehq.org 2011-12-19 07:43:22 CST --- There's no reason to make the server multithreaded. All blocking calls can be implemented with a non-blocking state machine, including calls to userspace, that BTW the server already supports. Please stop giving misguided implementation advice about things you clearly don't understand.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #49 from Adam Martinson adam.r.martinson@gmail.com 2011-12-19 12:05:01 CST --- Luke, I'm not sure we need a separate ncalrpc implementation, my patches do actually work fine with svcctl. Would be up to Alexandre though.
Alexandre, do you mind weighing in on the correct approach here?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #50 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-12-19 13:52:40 CST --- adam: it's fine... *if* you get it right. however if you get things wrong, you can't even run the test suite because wine won't _start_ the test suite, because starting up the tests (.exes) _require_ a working NamedPipes implementation in order to be able to call all regsvc and srvsvc functions.
come on, adam, i've pointed this out 3 times now. i apologise for not saying it clearly enough.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #51 from Adam Martinson adam.r.martinson@gmail.com 2011-12-19 13:59:43 CST --- (In reply to comment #50) Yeah I realize that wine won't start if this is botched, but I consider that a plus: there's a built-in test case xD
FYI Alexandre says the datagram-on-SOCK_STREAM approach won't work reliably with multiple threads sharing the same pipe, so I guess I'll have a go at the SOCK_SEQPACKET<->SOCK_STREAM approach.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #52 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-12-19 14:05:56 CST ---
There's no reason to make the server multithreaded. All blocking calls can be implemented with a non-blocking state machine, including calls to userspace, that BTW the server already supports.
i'm aware that it's a non-blocking state machine, alexandre. that's absolutely required, and it is good that it is a non-blocking state machine, alexandre.
however, unfortunately, in order to get a proper implementation of NamedPipes, it is necessary to use blocking calls on sockets.
that is simply and unavoidable a fact. there's no point in trying to deny this fact. it is simply a fact.
the only POSIX system call that can be used - period - is a blocking call on a socket.
fact.
now.
how, exactly, when there is only one thread / process in wineserver, which, if it is blocked because it is waiting for a response from a socket, will it be possible to service any other operations?
answer: more threads are needed... *in wineserver*.
so, please can i ask you the following:
1) you stop implying that i am a fucking moron. it'll get us nowhere, fast.
2) that you evaluate whether there are *any* other options - i don't believe that there are - any other POSIX functions and/or tricks available.
please feel free to entirely ignore both these requests. i don't have to put up with being told that i don't know what i'm talking about, and you should know by now not to do that.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #53 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-12-19 14:15:04 CST --- (In reply to comment #51)
(In reply to comment #50) Yeah I realize that wine won't start if this is botched, but I consider that a plus: there's a built-in test case xD
yes... but it's not a particularly good one! the diagnostics report is "Wine didn't work". or "Wine hung at startup".
what i'm saying is: if it's acceptable to have a completely inexplicable termination of wine's operation, then sure, that's "acceptable".
personally i would find that very very strange, to be working on a software (libre) project where it was acceptable to do this.
FYI Alexandre says the datagram-on-SOCK_STREAM approach won't work reliably with multiple threads sharing the same pipe,
preeciselyyyyyy.
well done.
now you know why i created one socket-pair *per message*. it's to avoid *exactly* that case where multiple threads try to read (or write) to a unix-pipe [as the underlying implementation]
... and that will result in resource-starvation.
keep going, ok? you'll get there. if you manage to make this work in the (critical) multi-threaded case, i'll be falling off my chair in shock.
what _would_ solve this would be if POSIX had a NamedPipe implementation. no, seriously! if the linux kernel had NamedPipe semantics, i.e. reliable SOCK_STREAM with datagram (fixed packet size) semantics *as well*, everything would be ok.
... but.... actually... it wouldn't be ok, because then the FreeBSD team would try to install Wine, and would find that it didn't work.
basically you're stuck with what's available in "old" style POSIX semantics.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #54 from Alexandre Julliard julliard@winehq.org 2011-12-19 15:25:28 CST --- (In reply to comment #52)
please feel free to entirely ignore both these requests. i don't have to put up with being told that i don't know what i'm talking about, and you should know by now not to do that.
I certainly know that arguing with you is a waste of time. My only reason for posting was to prevent others from thinking that they could get useful advice from this bug. I'll happily go back to ignoring you now.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #55 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-12-19 16:43:52 CST --- alexandre, feel free to do whatever you feel is most useful to you.
adam, i've just discovered this: http://urchin.earth.li/~twic/Sequenced_Packets_Over_Ordinary_TCP.html
a quick search online shows that SOCK_SEQPACKET is, indeed, only supported on a few "arcane" platforms, implemented on TCP on others but not on unix domain sockets, so its use would put wine at risk of completely failing to operate correctly... but only on certain platforms.
that link shows however that someone came up with a realllly weird usage for "urgent TCP pointers" which actually provides, amazingly, the same semantics.
about the only people that *i* know who would have a clue what that's about are some of the people i met when working at linuxcare. rusty russell, andrew tridgell, and a couple of others.
amazing. unix might actually have something that provides guaranteed atomic delivery of fixed-size messages. i've been hunting for that functionality for 12 years :)
http://bugs.winehq.org/show_bug.cgi?id=17195
David davilando@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |davilando@gmail.com
--- Comment #56 from David davilando@gmail.com 2011-12-21 15:21:47 CST --- First of all, thanks a lot for your hard work on the wine source code :D
I'm not sure if I'm adding anything valuable to help solving this issue, but I'll never know, until I try ;)
Since your are talking about named pipes, I thought the following bug reports are related to this bug report, and might help in testing the newly developed code/patches:
"Games with UPlay won't start due to ReadFileEx failture": http://bugs.winehq.org/show_bug.cgi?id=28119
"[Assassin's Creed II / Brotherhood]Online mode does not works": http://bugs.winehq.org/show_bug.cgi?id=28453
I've so, could you please add these bus as 'related to this bug'?
'And as affected apps' I think you can add a lot of Ubisoft Games: Assassins Creed 2 Assassins Creed Brotherhood Assassins Creed Revelations
Thanks!
And good luck fixing named pipes in wine!
http://bugs.winehq.org/show_bug.cgi?id=17195
Juan Lang juan.lang@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC|juan_lang@yahoo.com |
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #57 from Adam Martinson adam.r.martinson@gmail.com 2011-12-31 04:27:07 CST --- Got the SOCK_SEQPACKET<->SOCK_STREAM version mostly working, then discovered that named_pipe.c needs a total rewrite... Closing the server end of the pipe without disconnecting should behave the same way as the client end: reads on the other end are still perfectly valid... and that's incompatible with the way the current pipe objects are structured in the server. From what I can tell that's the only thing still stopping the python multithreading tests from succeeding. I'll post a new patch when I get that working.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #58 from Adam Martinson adam.r.martinson@gmail.com 2011-12-31 08:37:59 CST --- (In reply to comment #57) Hmm, maybe that's not the real issue... the python tests are giving me the same errors even with those mods. Could it be an implicit flush or something like that?? I'll have to write some tests... any ideas would be helpful.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #59 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2011-12-31 08:52:09 CST --- (In reply to comment #58)
(In reply to comment #57) Hmm, maybe that's not the real issue... the python tests are giving me the same errors even with those mods. Could it be an implicit flush or something like that?? I'll have to write some tests... any ideas would be helpful.
speak to the people that i advised you to speak to, and follow the links to the solution that i advised you to follow.
SOCK_SEQPACKET is *not* portable - its use will terminate any possibility of wine ever being useable on freebsd or anywhere other the absolute latest and greatest versions of gnu/linux OSes.
feel free to completely ignore this and any other advice and to learn the lesson that you feel is most useful to you.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #60 from Adam Martinson adam.r.martinson@gmail.com 2011-12-31 13:44:37 CST --- (In reply to comment #59)
SOCK_SEQPACKET is *not* portable - its use will terminate any possibility of wine ever being useable on freebsd or anywhere other the absolute latest and greatest versions of gnu/linux OSes.
I'm aware of this; on platforms/distros where SOCK_SEQPACKET isn't supported, we'll have to use TCP urgent pointers or something something like that. However, that's a step that can come later, first I'd like to get the thing working xD The TCP urgent pointers look like roughly equivalent functionality to SOCK_SEQPACKET, so for those platforms/distros we can replace those parts.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38025|0 |1 is obsolete| | Attachment #38026|0 |1 is obsolete| |
--- Comment #61 from Adam Martinson adam.r.martinson@gmail.com 2012-01-01 14:49:49 CST --- Created attachment 38210 --> http://bugs.winehq.org/attachment.cgi?id=38210 patch (work in progress)
OK, this one passes most of the python multithreading tests, but fails on sending a 16M message due to the system limit for SOCK_SEQPACKET.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #62 from Adam Martinson adam.r.martinson@gmail.com 2012-01-02 14:52:03 CST --- TCP urgent pointers aren't going to work; you only get 1 of them per socket end. From what I've read they're basically the TCP version of MSG_OOB. We'll have to figure something else out, I don't see Alexandre accepting this implementation due to the lack of portability.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #63 from Adam Martinson adam.r.martinson@gmail.com 2012-01-04 02:53:59 CST --- Alright, another idea, but a PITA to implement. This should be portable and give us the kind of low-level control we need to get correct behavior. Basically we implement the pipe's read/write buffers as shm segments protected by mutei. Then the server can just maintain the data stream internally as a list (or pair of lists for duplex). Huge PITA, as we'll have to make files for each pipe instance so we have something for ftok(), but should work.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38210|0 |1 is obsolete| |
--- Comment #64 from Adam Martinson adam.r.martinson@gmail.com 2012-01-07 19:05:16 CST --- Created attachment 38295 --> http://bugs.winehq.org/attachment.cgi?id=38295 patch (work in progress)
Ok, so Alexandre says we don't want to use shm; he suggested sending the messages through the server as part of the request/reply data, and... it works! It passes all of the python multiprocessing tests anyhow (tested 2.7 and 3.2).
However, there's a race condition with RTL_RWLOCKs somewhere, I'm not sure if it's related to my changes or not. In the python tests it manifests as an occasional hang at
test_lock (__main__.WithManagerTestLock) ...
In general I get
err:ntdll:RtlDeleteResource Deleting active MRSW lock (0x112414), expect failure
as the last message occasionally regardless of what wine is running, I'm guessing the 2 are related. Any insight into that would be helpful.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #65 from Dan Kegel dank@kegel.com 2012-01-07 19:17:01 CST --- I think I've been seeing that "Deleting active MRSW lock" error myself, so it might not be your changes.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #66 from Luca Bennati lucak3@gmail.com 2012-01-08 08:33:16 CST --- (In reply to comment #64)
In general I get err:ntdll:RtlDeleteResource Deleting active MRSW lock (0x112414), expect failure
As comment 65 by Dan Kegel says, not related to your changes, it already happens on vanilla Wine. It may just be that the python tests make Wine reach that code more often.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38295|0 |1 is obsolete| |
--- Comment #67 from Adam Martinson adam.r.martinson@gmail.com 2012-01-11 21:15:26 CST --- Created attachment 38329 --> http://bugs.winehq.org/attachment.cgi?id=38329 patch 1/4
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #68 from Adam Martinson adam.r.martinson@gmail.com 2012-01-11 21:15:52 CST --- Created attachment 38330 --> http://bugs.winehq.org/attachment.cgi?id=38330 patch 2/4
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #69 from Adam Martinson adam.r.martinson@gmail.com 2012-01-11 21:16:18 CST --- Created attachment 38331 --> http://bugs.winehq.org/attachment.cgi?id=38331 patch 3/4
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #70 from Adam Martinson adam.r.martinson@gmail.com 2012-01-11 21:17:59 CST --- Created attachment 38332 --> http://bugs.winehq.org/attachment.cgi?id=38332 patch 4/4
I could use some extra sets of eyes on these; any feedback would be helpful.
http://bugs.winehq.org/show_bug.cgi?id=17195
Dan grinnz@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |grinnz@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #71 from Jerome Leclanche adys.wh@gmail.com 2012-02-11 09:58:09 CST --- (In reply to comment #70) Adam, are you still interested in working on those patches after the 1.4 release?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #72 from Adam Martinson adam.r.martinson@gmail.com 2012-02-11 12:10:26 CST --- (In reply to comment #71)
(In reply to comment #70) Adam, are you still interested in working on those patches after the 1.4 release?
Yes; Alexandre said I should wait to submit them until then.
http://bugs.winehq.org/show_bug.cgi?id=17195
Luke Bratch l_bratch@yahoo.co.uk changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |l_bratch@yahoo.co.uk
http://bugs.winehq.org/show_bug.cgi?id=17195
surfing86 surfing86@live.it changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |surfing86@live.it
--- Comment #73 from surfing86 surfing86@live.it 2012-03-08 05:56:58 CST --- 1.4 version has been released I'm waiting for your updated patch, need it to start Anno 2070.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38329|0 |1 is obsolete| |
--- Comment #74 from Adam Martinson adam.r.martinson@gmail.com 2012-03-08 17:48:24 CST --- Created attachment 39250 --> http://bugs.winehq.org/attachment.cgi?id=39250 patch 1/4
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38330|0 |1 is obsolete| |
--- Comment #75 from Adam Martinson adam.r.martinson@gmail.com 2012-03-08 17:48:46 CST --- Created attachment 39251 --> http://bugs.winehq.org/attachment.cgi?id=39251 patch 2/4
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38331|0 |1 is obsolete| |
--- Comment #76 from Adam Martinson adam.r.martinson@gmail.com 2012-03-08 17:49:07 CST --- Created attachment 39252 --> http://bugs.winehq.org/attachment.cgi?id=39252 patch 3/4
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38332|0 |1 is obsolete| |
--- Comment #77 from Adam Martinson adam.r.martinson@gmail.com 2012-03-08 17:49:38 CST --- Created attachment 39253 --> http://bugs.winehq.org/attachment.cgi?id=39253 patch 4/4
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #78 from Adam Martinson adam.r.martinson@gmail.com 2012-03-08 17:52:35 CST --- (In reply to comment #73)
1.4 version has been released I'm waiting for your updated patch, need it to start Anno 2070.
I submitted them to winehq today. Getting them into wine may be a bit of a process, as they are big and affect sensitive areas, but you can use the updated ones here in the meantime.
http://bugs.winehq.org/show_bug.cgi?id=17195
Ernst ernst@planetcoms.co.za changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ernst@planetcoms.co.za
--- Comment #79 from Ernst ernst@planetcoms.co.za 2012-03-23 03:11:26 CDT --- Was hoping this would be implemented by now.
Sadly I cant seem to apply these patches to 1.4.
Any news from Wine Team regarding this?
http://bugs.winehq.org/show_bug.cgi?id=17195
Alexey Loukianov mooroon2@mail.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mooroon2@mail.ru
http://bugs.winehq.org/show_bug.cgi?id=17195
Saulius K. saulius2@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |saulius2@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
Daniel Jelinski djelinski1@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |djelinski1@gmail.com
--- Comment #80 from Daniel Jelinski djelinski1@gmail.com 2012-12-16 08:54:21 CST --- (In reply to comment #78)
I submitted them to winehq today. Getting them into wine may be a bit of a process, as they are big and affect sensitive areas, but you can use the updated ones here in the meantime.
Adam, are you still interested in getting these patches into Wine?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #81 from Adam Martinson adam.r.martinson@gmail.com 2012-12-16 13:17:25 CST --- (In reply to comment #80)
(In reply to comment #78)
I submitted them to winehq today. Getting them into wine may be a bit of a process, as they are big and affect sensitive areas, but you can use the updated ones here in the meantime.
Adam, are you still interested in getting these patches into Wine?
When I submitted them they were ignored, so unless Alexandre expresses an interest in getting them in I don't see it happening.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #82 from Daniel Jelinski djelinski1@gmail.com 2012-12-17 02:14:05 CST --- (In reply to comment #81)
When I submitted them they were ignored, so unless Alexandre expresses an interest in getting them in I don't see it happening.
Patch sequences are usually reviewed in order, and your first patch is way too large for reviewing. This is probably why the others were ignored. Patch 3 is is just about the right size. You are removing a comment in SetNamedPipeHandleState and adding a FIXME. You should probably add a note in your patch stating that you verified that the comment is no longer applicable. Tests from patch 2 should probably be moved into a separate patch. Other than that it looks good to me. Patch 4 also needs splitting. Keep in mind that usually the patches are reviewed, not the resulting code. As long as the code changes are obviously related to patch title, they should be okay.
Other than that, I was able to connect to MS SQL Server using named pipes protocol with these patches applied, so they are probably doing the right thing. I'd love to see them accepted. Will you split them?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #83 from Adam Martinson adam.r.martinson@gmail.com 2012-12-17 10:03:10 CST --- (In reply to comment #82)
Other than that, I was able to connect to MS SQL Server using named pipes protocol with these patches applied, so they are probably doing the right thing. I'd love to see them accepted. Will you split them?
It's easy enough to split the tests off into separate patches, but I could use some advice on how to split them up otherwise without breaking the "do something useful" and "don't break stuff" rules... Any ideas?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #84 from Daniel Jelinski djelinski1@gmail.com 2012-12-17 14:27:52 CST --- For starters, you are renaming several structs, constants and variables. This takes a lot of space in the patch, obfuscating the changes you care for. You should split the renames to a separate no-op patch, if you really need them. Patches that do not change behavior are easier to verify. Other than that, use git diff and try to make the result as small as possible for the patches that actually change something. Hopefully this will be enough to make the series verifiable.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #85 from Adam Martinson adam.r.martinson@gmail.com 2012-12-17 14:38:18 CST --- (In reply to comment #84)
For starters, you are renaming several structs, constants and variables. This takes a lot of space in the patch, obfuscating the changes you care for. You should split the renames to a separate no-op patch, if you really need them.
They're not essential, they just make it more clear how the pipe actually behaves. I'll give that a try though, thanks.
Any obvious issues with the ones from bug 17273 (SetNamedPipeHandleState)?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #86 from Daniel Jelinski djelinski1@gmail.com 2012-12-18 01:24:24 CST --- (In reply to comment #85)
Any obvious issues with the ones from bug 17273 (SetNamedPipeHandleState)?
The older version of patches 2 and 3 here? The new version looks better to me.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #87 from Adam Martinson adam.r.martinson@gmail.com 2012-12-18 12:53:57 CST --- (In reply to comment #86)
(In reply to comment #85)
Any obvious issues with the ones from bug 17273 (SetNamedPipeHandleState)?
The older version of patches 2 and 3 here? The new version looks better to me.
LOL, I guess they are (it's been a while since I looked at these) xD
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #88 from Adam Martinson adam.r.martinson@gmail.com 2012-12-18 19:52:08 CST --- Think I've got the first 3 split apart pretty well, any ideas on splitting the last one? I don't know how I would without breaking stuff in between...
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #89 from Daniel Jelinski djelinski1@gmail.com 2012-12-19 02:58:36 CST --- (In reply to comment #88)
Think I've got the first 3 split apart pretty well, any ideas on splitting the last one? I don't know how I would without breaking stuff in between...
Yes, it seems that you can't split this one. You can improve its readability in a different way: - in several places you are checking if the pipe is in message write mode. This check takes a lot of space, so you should probably put it into a function. - could you merge named_pipe_peek_msg with named_pipe_recv_msg? Or at least move common code to a function. This way if something breaks, you will have only one place to fix.
Also, this looks suspicious: + while (!(end->flags & NAMED_PIPE_MESSAGE_STREAM_READ) && + !list_empty( &end->msgs ) && + reply->result < req->length);
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #90 from Adam Martinson adam.r.martinson@gmail.com 2012-12-19 11:18:35 CST --- (In reply to comment #89)
- could you merge named_pipe_peek_msg with named_pipe_recv_msg? Or at least
move common code to a function. This way if something breaks, you will have only one place to fix.
I looked into that, doesn't seem reasonable as the behavior is rather different.
Also, this looks suspicious:
while (!(end->flags & NAMED_PIPE_MESSAGE_STREAM_READ) &&!list_empty( &end->msgs ) &&reply->result < req->length);
It's the correct behavior, if you're reading a byte stream you might keep going, but if you're reading a message you don't.
I'll upload my updated patches shortly, lemme know what you think.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #39250|0 |1 is obsolete| | Attachment #39251|0 |1 is obsolete| | Attachment #39252|0 |1 is obsolete| | Attachment #39253|0 |1 is obsolete| |
--- Comment #91 from Adam Martinson adam.r.martinson@gmail.com 2012-12-19 11:21:13 CST --- Created attachment 42851 --> http://bugs.winehq.org/attachment.cgi?id=42851 [PATCH 1/7] server/named_pipe: (no-op) Renames to make actual behavior clearer.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #92 from Adam Martinson adam.r.martinson@gmail.com 2012-12-19 11:21:57 CST --- Created attachment 42852 --> http://bugs.winehq.org/attachment.cgi?id=42852 [PATCH 2/7] server: Either end of a named pipe is readable after the other has been closed.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #93 from Adam Martinson adam.r.martinson@gmail.com 2012-12-19 11:22:31 CST --- Created attachment 42853 --> http://bugs.winehq.org/attachment.cgi?id=42853 [PATCH 3/7] kernel32: Tests for closing named pipes.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #94 from Adam Martinson adam.r.martinson@gmail.com 2012-12-19 11:22:55 CST --- Created attachment 42854 --> http://bugs.winehq.org/attachment.cgi?id=42854 [PATCH 4/7] ntdll: Add support for FILE_PIPE_INFORMATION.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #95 from Adam Martinson adam.r.martinson@gmail.com 2012-12-19 11:23:38 CST --- Created attachment 42855 --> http://bugs.winehq.org/attachment.cgi?id=42855 [PATCH 5/7] ntdll: Tests for FILE_PIPE_INFORMATION.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #96 from Adam Martinson adam.r.martinson@gmail.com 2012-12-19 11:24:39 CST --- Created attachment 42856 --> http://bugs.winehq.org/attachment.cgi?id=42856 [PATCH 6/7] kernel32: Add support for {Get|Set}NamedPipeHandleState().
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #97 from Adam Martinson adam.r.martinson@gmail.com 2012-12-19 11:25:22 CST --- Created attachment 42857 --> http://bugs.winehq.org/attachment.cgi?id=42857 [PATCH 7/7] server: Add support for named pipe message mode.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #42854|0 |1 is patch| | Attachment #42854|application/octet-stream |text/plain mime type| |
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #98 from Daniel Jelinski djelinski1@gmail.com 2012-12-19 15:31:53 CST --- (In reply to comment #90)
It's the correct behavior, if you're reading a byte stream you might keep going, but if you're reading a message you don't.
Sorry, I misread the code. My mistake.
I'll upload my updated patches shortly, lemme know what you think.
- Drop the whitespace changes from patches 2, 3 and 6 - get rid of warnings - I got one: file.c: In function ‘FILE_AsyncReadService’: file.c:383:12: warning: variable ‘hAvail’ set but not used [-Wunused-but-set-variable] - and send to wine-patches :) Hopefully this time you'll get better response.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #99 from Adam Martinson adam.r.martinson@gmail.com 2012-12-19 16:48:15 CST --- (In reply to comment #98)
- Drop the whitespace changes from patches 2, 3 and 6
- get rid of warnings - I got one:
file.c: In function ‘FILE_AsyncReadService’: file.c:383:12: warning: variable ‘hAvail’ set but not used [-Wunused-but-set-variable]
- and send to wine-patches :)
Hopefully this time you'll get better response.
Sent, wish me luck, and thanks for the help =)
http://bugs.winehq.org/show_bug.cgi?id=17195
anatoly techtonik techtonik@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |techtonik@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
Anastasius Focht focht@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |focht@gmx.net
http://bugs.winehq.org/show_bug.cgi?id=17195
euug3576+winebugs@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |euug3576+winebugs@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #100 from Adam Martinson adam.r.martinson@gmail.com 2012-12-20 14:01:01 CST --- Well, they got looked at this time =) The 2nd patch still needs splitting though, any ideas?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #101 from Adam Martinson adam.r.martinson@gmail.com 2012-12-20 14:32:53 CST --- (In reply to comment #100)
Well, they got looked at this time =) The 2nd patch still needs splitting though, any ideas?
I split the ntdll + kernel32 parts off, I guess they're more to do with zero-length pipe reads than anything else. Think that will be enough?
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #42851|0 |1 is obsolete| | Attachment #42852|0 |1 is obsolete| | Attachment #42853|0 |1 is obsolete| | Attachment #42854|0 |1 is obsolete| | Attachment #42855|0 |1 is obsolete| | Attachment #42856|0 |1 is obsolete| | Attachment #42857|0 |1 is obsolete| |
--- Comment #102 from Adam Martinson adam.r.martinson@gmail.com 2012-12-20 14:46:57 CST --- Created attachment 42872 --> http://bugs.winehq.org/attachment.cgi?id=42872 Patches (8)
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #103 from Daniel Jelinski djelinski1@gmail.com 2012-12-20 14:50:51 CST --- It's still not quite clear what exactly that patch changes. Am I right that the main part is adding pipe_end_flush and pipe_end_ioctl, and making both pipe ends use the same set of fd_ops? If so, you could split the cosmetic changes (i.e. everything else) into a separate patch after that one. Yes, that probably means you will need to add some code in one patch only to delete it in the next one, but that should improve readability considerably.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #42872|1 |0 is patch| | Attachment #42872|text/plain |application/octet-stream mime type| |
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #42872|application/octet-stream |application/x-gzip mime type| |
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #104 from Adam Martinson adam.r.martinson@gmail.com 2012-12-20 16:10:55 CST --- (In reply to comment #103)
It's still not quite clear what exactly that patch changes. Am I right that the main part is adding pipe_end_flush and pipe_end_ioctl, and making both pipe ends use the same set of fd_ops? If so, you could split the cosmetic changes (i.e. everything else) into a separate patch after that one. Yes, that probably means you will need to add some code in one patch only to delete it in the next one, but that should improve readability considerably.
Sort of. Basically both ends of the pipe behave more or less the same, so it moves the stuff that's shared into the pipe_server object and the stuff that's specific to each end into the pipe_end object. Perhaps I should rename pipe_server to pipe_instance in the 1st patch? I could also and an enum for which end is which (enum { pe_server, pe_client };).
The major change becomes using the pipe_end object for both the client and server ends, and I don't think there's any way to do that without changing it everywhere at once. The addition of pipe_end_ioctl() really just takes over FSCTL_PIPE_DISCONNECT from pipe_server_ioctl(), because pipe_server_ioctl will only be called if the pipe isn't connected. pipe_end_flush() is really just a rename of pipe_server_flush(), and becomes usable from either end.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #105 from Daniel Jelinski djelinski1@gmail.com 2012-12-21 02:58:03 CST --- (In reply to comment #104)
(..) Perhaps I should rename pipe_server to pipe_instance in the 1st patch?
Yes, I think that's a good idea.
I could also and an enum for which end is which (enum { pe_server, pe_client };).
Or use: struct pipe_end *server, *client; instead of: struct pipe_end *ends[2]; That could also help.
The major change becomes using the pipe_end object for both the client and server ends, and I don't think there's any way to do that without changing it everywhere at once.
Well, the patch title says that the main change is making both pipe ends readable. Is there any feasible way to separate making both ends readable from making both ends use pipe_end? If there is, you should probably do it. If not (and it seems this way), you should probably change the patch title to reflect the major change, and add a comment that the side effect is making both ends readable. Also I noticed that you changed the order of create_pipe_server and create_pipe_end. If you restore the original order, you'll get a smaller diff :) There's not much more you can do here, so that must suffice.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #42872|0 |1 is obsolete| |
--- Comment #106 from Adam Martinson adam.r.martinson@gmail.com 2012-12-21 09:43:54 CST --- Created attachment 42882 --> http://bugs.winehq.org/attachment.cgi?id=42882 Patches (9)
Using an array for the ends makes the relay logic easier in the later patches, so I think I'll stick with that. How do these look?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #107 from Adam Martinson adam.r.martinson@gmail.com 2012-12-21 10:15:59 CST --- (In reply to comment #105)
Also I noticed that you changed the order of create_pipe_server and create_pipe_end. If you restore the original order, you'll get a smaller diff :)
I need to call create_pipe_end() in create_pipe_instance(), so I would have to forward-declare it otherwise.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #108 from Daniel Jelinski djelinski1@gmail.com 2012-12-22 01:57:45 CST --- These look good to me. Get your defense speech ready and send them in :)
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #109 from Adam Martinson adam.r.martinson@gmail.com 2012-12-28 15:13:22 CST --- (In reply to comment #108)
These look good to me. Get your defense speech ready and send them in :)
Well, I tried; looks like they're back to being ignored. If you can get Alexandre to tell you what's wrong I'm happy to fix it, but without knowing what the problem is I don't think there's anything more to be done.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #110 from Daniel Jelinski djelinski1@gmail.com 2012-12-29 02:32:40 CST --- Alexandre finally sent a comment: http://www.winehq.org/pipermail/wine-devel/2012-December/098271.html I think you may have to break some things in order to split patch 3. From my experience, it may be acceptable to break something in a patch to fix it in a later one, as long as 1) you describe what exactly is broken and which patch fixes it, and 2) you don't break make test. I never had to add todo_wine, so I don't know if that would pass.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #42882|0 |1 is obsolete| |
--- Comment #111 from Adam Martinson adam.r.martinson@gmail.com 2012-12-29 13:21:13 CST --- Created attachment 43008 --> http://bugs.winehq.org/attachment.cgi?id=43008 Patches (9)
I've got patch 3 cut down a little more, think that's enough?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #112 from Daniel Jelinski djelinski1@gmail.com 2012-12-29 14:51:47 CST --- I like it better, but I don't think it will be enough. I think you have to remove/comment out all fd_ops you need to change, and then re-add them one at a time.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #113 from Adam Martinson adam.r.martinson@gmail.com 2012-12-29 23:33:56 CST --- (In reply to comment #112)
I like it better, but I don't think it will be enough. I think you have to remove/comment out all fd_ops you need to change, and then re-add them one at a time.
But I can't actually break stuff here, because if I do wine will not boot properly and so it can't run the tests anyhow. How do I split it without breaking stuff?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #114 from Daniel Jelinski djelinski1@gmail.com 2012-12-30 03:38:27 CST --- (In reply to comment #113)
How do I split it without breaking stuff?
I have no idea. You should ask it on wine-devel, going through your available options: - splitting to smaller patches that break stuff, hoping that AJ will re-merge before applying instead of just rejecting them - dropping that patch from your series - making CloseHandle work correctly in one patch, then splitting used structures - waiting for someone to come up with a better idea, which you have already been doing for over a year.
If I remember correctly, options 2 and 3 would result in even bigger unsplittable patches, so the first one looks best to me, but ask AJ first.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #115 from euug3576+winebugs@gmail.com 2012-12-30 04:20:13 CST --- Whilst I don't have the expertise to help you guys technically, I'd like to say thanks on behalf of wine users for your time & persistence!!
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #43008|0 |1 is obsolete| |
--- Comment #116 from Adam Martinson adam.r.martinson@gmail.com 2012-12-30 15:45:30 CST --- Created attachment 43034 --> http://bugs.winehq.org/attachment.cgi?id=43034 Patches (9)
Re-uploading these, think I zipped them up wrong.
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #43034|0 |1 is obsolete| |
--- Comment #117 from Adam Martinson adam.r.martinson@gmail.com 2012-12-31 14:56:27 CST --- Created attachment 43046 --> http://bugs.winehq.org/attachment.cgi?id=43046 Patches (12)
Got that one down to 20k by moving some trivial stuff to separate patches. Worth resubmitting yet or no?
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #43046|0 |1 is obsolete| |
--- Comment #118 from Adam Martinson adam.r.martinson@gmail.com 2013-01-02 19:09:12 CST --- Created attachment 43063 --> http://bugs.winehq.org/attachment.cgi?id=43063 Patches (10)
Dan, I tried your splitting idea, and it looks pretty good to me. Any thoughts?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #119 from Daniel Jelinski djelinski1@gmail.com 2013-01-03 01:52:54 CST --- You can (and should) split it some more. Changing flush deserves its separate patch, and so does changing ioctl. Also this series is getting quite long. You could stop resending the later patches to wine-patches until at least some get accepted.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #120 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2013-01-03 04:00:28 CST --- there's a way round the "requirement" that tests be run, which is to add unnecessary code that checks - at runtime - whether the old functionality is to be used or the new functionality is to be used, and to have the choice be made either as a compile-time switch or even as a configuration option.
in this way, the code gets massively complicated and it would be a hell of a lot of extra work for adam, as well as for all reviewers concerned, but the "piecemeal" patch could be applied, because each patch could be applied individually, knowing that it is going to have zero effect.
another alternative to that is to actually patch in replacement functions with alternative names, where the replacement functions are not called (at all) until the last and final patch.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #121 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2013-01-03 04:01:06 CST --- btw adam did you also check that this patch works on FreeBSD?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #122 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2013-01-03 04:09:32 CST --- i'm going to suggest that this and other renames be made as separate patches. it's very unclear that there's anything significant to review here, because the only thing that's happening is pipe_server and pipe_instance are being renamed, and server to end.
oh wait! event is also being renamed to event_empty which is highly significant, and buried within the renaming.
-static void notify_empty( struct pipe_server *server ) +static void notify_empty( struct pipe_instance *end ) { - if (!server->flush_poll) + if (!end->flush_poll) return; - assert( server->state == ps_connected ); - assert( server->event ); - remove_timeout_user( server->flush_poll ); - server->flush_poll = NULL; - set_event( server->event ); - release_object( server->event ); - server->event = NULL; + assert( end->state == ps_connected ); + assert( end->event_empty ); + remove_timeout_user( end->flush_poll ); + end->flush_poll = NULL; + set_event( end->event_empty ); + release_object( end->event_empty ); + end->event_empty = NULL; }
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #123 from Adam Martinson adam.r.martinson@gmail.com 2013-01-03 10:00:18 CST --- (In reply to comment #119)
You can (and should) split it some more. Changing flush deserves its separate patch, and so does changing ioctl.
You mean split patch 5 more?
(In reply to comment #120)
there's a way round the "requirement" that tests be run, which is to add unnecessary code that checks - at runtime - whether the old functionality is to be used or the new functionality is to be used, and to have the choice be made either as a compile-time switch or even as a configuration option.
I don't think Alexandre would go for that without some compelling reason.
(In reply to comment #121)
btw adam did you also check that this patch works on FreeBSD?
No, but there's no reason it shouldn't. I'm not using any system calls, etc that Wine doesn't already use.
(In reply to comment #122)
oh wait! event is also being renamed to event_empty which is highly significant, and buried within the renaming.
Why is that significant? Empty events are the only thing it's used for. In a later patch an event_avail gets added, I just renamed it to avoid confusion. The structs in here are just for the server, the renames don't affect anything outside of that file.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #124 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2013-01-03 12:48:16 CST ---
(In reply to comment #120)
there's a way round the "requirement" that tests be run, which is to add unnecessary code that checks - at runtime - whether the old functionality is to be used or the new functionality is to be used, and to have the choice be made either as a compile-time switch or even as a configuration option.
I don't think Alexandre would go for that without some compelling reason.
well, alexandre already asked for tests to be carried out, and you've already explained that tests cannot be carried out unless the *entire* patch set is applied and accepted, because it's so fundamental.
the two things are therefore mutually exclusively incompatible, therefore a third way is essential to devise. and accept that it is necessary to devise.
a runtime switch or a compile time switch is such a third way.
if that third way also turns out to be unacceptable then, round the loop we go: one of the other two choices must be chosen... or a fourth way found.
(In reply to comment #121)
btw adam did you also check that this patch works on FreeBSD?
No, but there's no reason it shouldn't. I'm not using any system calls, etc that Wine doesn't already use.
ok great! last time you mentioned using some weird feature of TCP that was only available to linux.
(In reply to comment #122)
oh wait! event is also being renamed to event_empty which is highly significant, and buried within the renaming.
Why is that significant? Empty events are the only thing it's used for. In a later patch an event_avail gets added, I just renamed it to avoid confusion. The structs in here are just for the server, the renames don't affect anything outside of that file.
it's not obvious just from the patch what's going on. is it *only* renaming? if so, what gets renamed to what? if you only renamed one at a time, the number of lines being changed would be smaller, so comparisons would be easy.
the sheer number of things being renamed here actually removes *entire* functions and replaces them with ones that are... identical? are they? i don't know, and i can't tell.
later on in the patch, the renaming becomes sufficiently severe that it looks like you're removing create_pipe_server. whereas what it looks like is that you've swapped the function order of find_available_[server/instance] and create_pipe_[server/instance]. i don't know, because it's hard to tell.
my recommendation would be - pain in the ass as it is - to do this as completely separate renames. and for each patch to contain the renaming of the header file as well as all files in which each renamed function/member variable is reused.
this would at least allow those to be done as entirely independent separate patches that would clearly have nothing to do with the actual functionality.
they could in fact be done under a separate bug report, so as to shorten the length of this one.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #125 from Daniel Jelinski djelinski1@gmail.com 2013-01-03 15:13:35 CST --- (In reply to comment #123)
(In reply to comment #119)
You can (and should) split it some more. Changing flush deserves its separate patch, and so does changing ioctl.
You mean split patch 5 more?
Yes. If some app breaks after your commits, you'll get a regression test pointing to one of them. The smaller that patch is, the better for you. Apps tend to break on good commits too, so even if you're sure everything is OK, splitting is always a good idea.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #126 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2013-01-03 15:30:22 CST --- (In reply to comment #125)
(In reply to comment #123)
(In reply to comment #119)
You can (and should) split it some more. Changing flush deserves its separate patch, and so does changing ioctl.
You mean split patch 5 more?
Yes. If some app breaks after your commits, you'll get a regression test pointing to one of them. The smaller that patch is, the better for you. Apps tend to break on good commits too, so even if you're sure everything is OK, splitting is always a good idea.
daniel: this is a... a... severely fundamental and critical functional patch which should have been done a long, *long* time ago. i'm staggered that the NamedPipes functionality wasn't designed right from the start (and i mean when the very first implementation of NamedPipes was ever conceived) to be properly compliant with the NamedPipes specification.
how in hell's teeth's name wine got as far as it has without having proper datagram support in NamedPipes is an absolute mystery.
the point is: as adam points out, this is very much an all-or-nothing patch. if it works, it will "just work". if it fails, it will fail so spectacularly and so early that it will be blindingly f*****g obvious, it's *that* fundamental.
NamedPipes are used at the absolute most fundamental level by the infrastructure of wine. \PIPE\svcctl for example - to start even the very first services? registry access? all done through named pipes (\PIPE\winreg).
if this patch didn't do its job, wine wouldn't work... period.
if with this patch you can start wine, it's called so many times even at the very startup that that is, in its own way, its own test.
the patch is so fundamental to wine that not even the tests that i created which test the functionality of this patch can be run unless the functionality itself has been correctly implemented in the patch.
after startup, if there is even one single 3rd party program that fails because of this patch, i would actually be more inclined to start any investigation with that application more than i would investigate this patch as the cause.
i'm emphasising the same point in several different ways, but is it clear that this is very much an all-or-nothing patch which either works 100% or fails spectacularly 100%?
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #127 from Adam Martinson adam.r.martinson@gmail.com 2013-01-03 21:04:20 CST --- (In reply to comment #125)
(In reply to comment #123)
You mean split patch 5 more?
Yes. If some app breaks after your commits, you'll get a regression test pointing to one of them. The smaller that patch is, the better for you. Apps tend to break on good commits too, so even if you're sure everything is OK, splitting is always a good idea.
I know that splitting is good xD But that's the one where you stop getting a pipe_instance handle and start getting a pipe_end handle for the server end. Those functions *must* change at the same time or stuff breaks. Is there a way to get around that that I'm not seeing?
(In reply to comment #124)
well, alexandre already asked for tests to be carried out, and you've already explained that tests cannot be carried out unless the *entire* patch set is applied and accepted, because it's so fundamental.
They don't all have to be committed at once. The 1st one can stand on its own. 2-6 are the 1st chunk, redesigning the backend. 7-9 are the 2nd chunk, implementing {Get|Set}NamedPipeHandleState() and the FILE_PIPE_INFORMATION stuff. 10 is the last chunk, I just don't know how to split that one without breaking stuff.
He didn't have any problem with the tests that I recall, he just wanted the major patch from the 1st chunk split AFAIK, so I don't think a switch is needed.
it's not obvious just from the patch what's going on. is it *only* renaming? if so, what gets renamed to what? if you only renamed one at a time, the number of lines being changed would be smaller, so comparisons would be easy.
The patches that say (no-op) are only renames. part 1: ps_connected_server => ps_connected ps_wait_disconnect => ps_disconnected_client client => end part 2: instances => numinstances server => instance servers => instances event => event_empty
the sheer number of things being renamed here actually removes *entire* functions and replaces them with ones that are... identical? are they? i don't know, and i can't tell.
If you use a diff tool that shows changes inside lines it's easier. I'd rather not create a whole bunch of no-op patches unless Alexandre thinks it's necessary.
this would at least allow those to be done as entirely independent separate patches that would clearly have nothing to do with the actual functionality.
That's what the (no-op) in the description is for ;-)
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Martinson adam.r.martinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #43063|Patches (10) |Patches (10) - ignore the description| |duplicate 0005-* (oops)
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #128 from Daniel Jelinski djelinski1@gmail.com 2013-01-04 00:12:02 CST --- (In reply to comment #127)
I know that splitting is good xD But that's the one where you stop getting a pipe_instance handle and start getting a pipe_end handle for the server end. Those functions *must* change at the same time or stuff breaks. Is there a way to get around that that I'm not seeing?
Make pipe_instance_flush read: { if(inst->server->fd) { pipe_end_flush(inst->server); return; } no_flush(); } or something along these lines. Same for pipe_instance_ioctl. When you switch to pipe_end_fd_ops, these lines will never be called, so you may remove them in a later patch (which can even be a separate one).
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #129 from Adam Martinson adam.r.martinson@gmail.com 2013-01-04 00:20:42 CST --- (In reply to comment #128)
or something along these lines. Same for pipe_instance_ioctl. When you switch to pipe_end_fd_ops, these lines will never be called, so you may remove them in a later patch (which can even be a separate one).
That works for flush I suppose, ioctl will be a little trickier but I think it should be doable.
http://bugs.winehq.org/show_bug.cgi?id=17195
rmlipman@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |rmlipman@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
trilavia@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |trilavia@gmail.com
--- Comment #130 from trilavia@gmail.com 2013-01-20 12:10:06 CST --- Any chance for ubuntu 12.10 wine 1.5 package patched for this bug? I can't compile wine on my system. Thanks in advance!
http://bugs.winehq.org/show_bug.cgi?id=17195
nE0sIghT update.microsoft@mail.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |update.microsoft@mail.ru
http://bugs.winehq.org/show_bug.cgi?id=17195
Jonny Barnes jonnybarnes@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jonnybarnes@gmail.com
--- Comment #131 from Jonny Barnes jonnybarnes@gmail.com 2013-03-30 11:38:02 CDT --- This appears to have stopped working. I'm ignoring the `0005-server-Use-the-same-fd-ops-for-both-ends-of-a-named-.patch` patch file.
Once I've cloned the git repository (--depth 1 if that makes anydifference) and start applying patches, it fails at patch 7. The output from `patch -p1 < 0007._.patch` is located at http://pastebin.ca/2345630
The contents of include/wine/server_protocol.h.rej is:
--- include/wine/server_protocol.h +++ include/wine/server_protocol.h @@ -5729,6 +5745,6 @@ struct set_suspend_context_reply set_suspend_context_reply; };
-#define SERVER_PROTOCOL_VERSION 437 +#define SERVER_PROTOCOL_VERSION 438
#endif /* __WINE_WINE_SERVER_PROTOCOL_H */
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #132 from Dan Kegel dank@kegel.com 2013-03-30 12:33:24 CDT --- If that's the only problem, you can just increment the server protocol by hand with a text editor.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #133 from Jonny Barnes jonnybarnes@gmail.com 2013-03-30 12:46:40 CDT --- (In reply to comment #132)
If that's the only problem, you can just increment the server protocol by hand with a text editor.
Looking at the latest git source, that line is now
#define SERVER_PROTOCOL_VERSION 440
should I change that to 437 to match the patch file, or edit the patch file to change 440 to 441 or what? I have no idea what SERVER_PROTOCOL_VERSION does.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #134 from Dan Kegel dank@kegel.com 2013-03-30 12:54:25 CDT --- It just keeps programs built against new wine from trying to run on old wine. Just make it one higher than anything ever seen before. Or set it to 9999.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #135 from Jonny Barnes jonnybarnes@gmail.com 2013-03-30 14:39:44 CDT --- (In reply to comment #134)
It just keeps programs built against new wine from trying to run on old wine. Just make it one higher than anything ever seen before. Or set it to 9999.
Working, thanks Dan.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #136 from Henri Verbeet hverbeet@gmail.com 2013-04-01 07:15:20 CDT --- Just for what it's worth, you should just about never patch server_protocol.h. It's supposed to be generated from protocol.def by tools/make_requests.
http://bugs.winehq.org/show_bug.cgi?id=17195
Kyle Devir kyle.devir@gmx.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle.devir@gmx.com
http://bugs.winehq.org/show_bug.cgi?id=17195
vegarh@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |vegarh@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
napoliste@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |napoliste@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |33583
http://bugs.winehq.org/show_bug.cgi?id=17195
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |joe_rocks@hotmail.com
--- Comment #137 from Alexandre Julliard julliard@winehq.org 2013-05-25 16:26:07 CDT --- *** Bug 33656 has been marked as a duplicate of this bug. ***
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #138 from Berillions berillions@gmail.com 2013-05-30 15:38:37 CDT --- Impossible to apply the patch #10 with the actual wine git version wine-1.5.31-97-gc6d3075
http://bugs.winehq.org/show_bug.cgi?id=17195
Kjetil Ness nesskjetil@hotmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |nesskjetil@hotmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
Kody khicks545@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |khicks545@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
Andrey andrey.gursky@e-mail.ua changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |andrey.gursky@e-mail.ua
--- Comment #139 from Andrey andrey.gursky@e-mail.ua 2013-07-18 09:58:02 CDT --- (In reply to comment #138)
Impossible to apply the patch #10 with the actual wine git version wine-1.5.31-97-gc6d3075
Patch 0007 alters include/wine/server_protocol.h and 0010 too. The later is unneeded.
For 1.6rc5 you can edit the patch 0007 and replace as foolows:
-#define SERVER_PROTOCOL_VERSION 440 +#define SERVER_PROTOCOL_VERSION 999
In the patch 0010 you can either remove these changes or just replace as follows:
-#define SERVER_PROTOCOL_VERSION 999 +#define SERVER_PROTOCOL_VERSION 999
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #140 from Andrey andrey.gursky@e-mail.ua 2013-07-18 10:05:37 CDT --- I've tried the patches with 1.6rc5: they compiled well but the installation of vcredist_x86_2012 failed.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #141 from Andrey andrey.gursky@e-mail.ua 2013-07-18 10:06:36 CDT --- Created attachment 45327 --> http://bugs.winehq.org/attachment.cgi?id=45327 wine 1.6rc5 shell output
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #142 from Andrey andrey.gursky@e-mail.ua 2013-07-18 10:07:31 CDT --- Created attachment 45328 --> http://bugs.winehq.org/attachment.cgi?id=45328 vcredist_x86_2012 installation log
http://bugs.winehq.org/show_bug.cgi?id=17195
Luis Zaldivar luis.zaldivar@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |luis.zaldivar@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
Amine amineleblaugrana@hotmail.fr changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |amineleblaugrana@hotmail.fr
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #143 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2013-07-30 19:47:18 CDT --- andrey: that means that there's a case which hasn't been tested for, yet. tracking it down is gonna be fuuun.
could i ask you please to do a "vanilla" (unpatched) install log and to then go through the two log files, doing a side-by-side compare to find the most likely discrepancy that could have caused the failure on the patched version?
it should be pretty obvious: this is pretty fundamental stuff: NamedPipes is at an absolute critical low-level in wine and w32 so it should be blindingly obvious which call is failing, it's just finding the right one and that means doing a side-by-side compare.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #144 from Luke Kenneth Casson Leighton lkcl@lkcl.net 2013-07-30 19:50:22 CDT --- apologies, andrey: i meant "wine debug logs", not vcredist install logs.
if you could kindly run (in completely new/fresh installs) enabling wine debug logging on each install, one patched and one unpatched.
you're looking for debug messages from the NamedPipe create, writes, reads etc. you might have to ask on the wine mailing list for help on how to enable wine debug logging without completely overwhelming yourself! it's been a while since i did this so i can't recall exactly how it's done.
http://bugs.winehq.org/show_bug.cgi?id=17195
oli.huber@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |oli.huber@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
Adam Dempsey dempsey@weirdfish.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dempsey@weirdfish.net
https://bugs.winehq.org/show_bug.cgi?id=17195
Anastasius Focht focht@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17273 Depends on|17273 |
https://bugs.winehq.org/show_bug.cgi?id=17195
Sylvain Petreolle spetreolle@yahoo.fr changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |spetreolle@yahoo.fr
http://bugs.winehq.org/show_bug.cgi?id=17195
Anastasius Focht focht@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jeanluc.malet@gmail.com
--- Comment #145 from Anastasius Focht focht@gmx.net --- *** Bug 21840 has been marked as a duplicate of this bug. ***
https://bugs.winehq.org/show_bug.cgi?id=17195
Anastasius Focht focht@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dmbohdan@gmail.com
--- Comment #146 from Anastasius Focht focht@gmx.net --- *** Bug 23914 has been marked as a duplicate of this bug. ***
https://bugs.winehq.org/show_bug.cgi?id=17195
jeanluc.malet@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC|jeanluc.malet@gmail.com |
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #147 from Luke Kenneth Casson Leighton lkcl@lkcl.net --- guys,
the work adding proper named pipes support is now five years old. a large number of programs critically depend on this working. there have been four other duplicate bugs and there are two blockers, one of which has *eighteen* further duplicates.
i did not tackle this particular functionality without a good reason, because i know how critical and low-level it is. i am amazed that it was not tackled over a decade ago when wine was first starting out.
now, one of the problems is that because it is so low-level, it is actually very hard to even get wine executables started to the point where tests can be run.
to make that clear: this is such low-level functionality that even *STARTING WINE* is itself a pretty much full and complete test.
to reiterate: by the time you get to the point where the test executables can *be run*, there will have *already* been hundreds of function calls involving NamedPipe MessageMode datagrams.
additionally, i already pointed out that the use of sockets is not very appropriate. the GNU/Linux OS and the BSD OSes simply do not have the fundamental OS-level concept of reliable streamed datagrams, and emulating them in a single-threaded environment (wineserver) is a bitch and a half.
also, i have pointed out that to use sockets you need to make wineserver multi-threaded. mr alexandre jiullard has told everyone that everything that i say is total shit, and i am not interested in educating mr jiullard.
also, i have pointed out that if you keep using a single-threaded wineserver the only other way to properly implement reliable streamed datagrams (in a fast manner) is to put the data into shared memory (then use fixed-length (semaphored) message encoding via wineserver to communicate between user-level processes where that data may be obtained). mr jiullard has also said that this idea is shit. i am not intereested in educating mr jiullard.
you _could_ implement streamed reliable datagrams in a single-threaded wineserver by placing the datagram's data into files (one file per packet) and, again, use the single-threaded wineserver to communicate with a fixed-length (semaphore-protected) message where the datagram's data could be obtained. however this _would_ actually be shit, because it would be unbelievably slow.
so the summary is: we basically know where the problem lies, and it is in the attitude of mr jiullard. he doesn't like the fact that i have expertise in this area. until he gets over his psychiatric problem this bug is going to continue to be blocked and a huge number of applications - some of them quite prominent - will be excluded from the list of applications that wine is capable of running.
http://bugs.winehq.org/show_bug.cgi?id=17195
Brandon Corujo haku08879@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |haku08879@gmail.com
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #148 from Adam Martinson adam.r.martinson@gmail.com --- (In reply to Luke Kenneth Casson Leighton from comment #147)
...rant...
Luke, if you really are the expert you seem to think you are, help me figure out why the patches which use Julliard's proposed solution cause problems in some apps that work fine without them (see the comments in Bug 17273). You want this fixed? That's how you can help.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #149 from Luke Kenneth Casson Leighton lkcl@lkcl.net --- hi adam,
i've been investigating alternative options. one of them is posix ipc queues. posix ipc queues have exactly the same semantics (itliterally) as message-mode named pipes. there are however a number of problems with posix ipc queues. firstly, they tend not to have been implemented correctly. secondly, clean-up is a bitch. there is typically no way to *enumerate* the ipc queues despite them being like files! thirdly, the limit is around 8k whereas namedpipe in messagemode is something like a 64k message size limit.
so, using posix ipc queues would be a bit fraught, but technically would be [one of the very few] the correct things to use, even as a means to pass e.g. just the length of the message to be read from the socket. what is nice about posix ipc queues is that they have an equivalent of poll, so you can tell whether there is a message waiting, at least, or, better, use them to block the user process (pending a message), instead of blocking the (single) wineserver process (which you would otherwise have to multi-thread in order to get the correct semantics required, here).
but, first, we have to get jiullard to stop putting pride, anger and hate ahead of technical analysis and decisions. someone else (other than me) basically needs to stand up to him.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #150 from Dan Kegel dank@kegel.com --- Luke, Alexandre is the maintainer of the winehq tree. It's his (paid!) job, and he's good at it.
As Adam says, your best move for now is to let the wookie win, and help debug Adam's existing patchset.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #151 from Luke Kenneth Casson Leighton lkcl@lkcl.net --- hi dan,
i had time, back when i was working on this. if i had received encouraging responses from jiullard instead of one-sentence "you're a dick, fuck off" style responses then i would have worked hard - at the time - to come up with a solution.
so jiullard needs to learn - and everyone else needs to understand - that he is *not* good at what he does.
if i was paid to work on wine, then *regardless* of my personal opinion of an individual's nature if they showed the slightest inclination and enthusiasm to help out on such a complex project i would not hesitate to encourage them to do so, dealing with anything where they stepped out of line as a separate issue.
we know that NamedPipes is a highly complex area that should have been tackled a decade ago. there are very few people in the world who a) understand low-level NT b) understand low-level POSIX/Unix.
so i am simply not prepared to put up with shit - for no money - in such a complex technical area.
discouraging people from contributing is massively counterproductive.
get him to publicly apologise and i might consider helping out again.
but otherwise? why should i bother? i have enough to do without having to put up with his attitude.
so, sorry dan - no dice.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #152 from Adam Martinson adam.r.martinson@gmail.com --- (In reply to Luke Kenneth Casson Leighton from comment #151)
...rant...
Filling up this page with the details of why Alexandre pisses you off doesn't count as helping; it just adds useless text to wade through for anyone trying to work on this bug.
https://bugs.winehq.org/show_bug.cgi?id=17195
Simon swdevelop1981@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |swdevelop1981@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=17195
Jonas webforlife@outlook.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |webforlife@outlook.com
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #153 from Jonas webforlife@outlook.com --- I don't know much about fixing the bug, but it is quite serious. This bug is Blocking http://bugs.winehq.org/show_bug.cgi?id=17273 which means that anything and everything using Microsoft Visual C++ 2012 or newer will not work in wine since the 2012 redist package halts and shuts down wine.
So if there is anything that I can to do help fixing it, logs, tests, ANYTHING, please let me know.
http://bugs.winehq.org/show_bug.cgi?id=17195
Sebastian Lackner sebastian@fds-team.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |sebastian@fds-team.de
--- Comment #154 from Sebastian Lackner sebastian@fds-team.de --- Since noone seems to be working on this anymore, we (= the Wine-compholio team) decided to give it another try. ;-) First of all, when taking a look at all the approaches which have been there so far, there are immediately two things I've noticed:
* At my opinion its unlikely that AJ will accept a solution, which involves the wineserver roundtrip time. It would decrease performance significantly compared to all the previous wine versions, and so is probably not a good thing...
* Even if some of the implementations are probably (mostly) correct, the tests are not sufficient to show that. A lot of corner cases are not tested at all, for example:
- partial receiving of messages - partial receiving from multiple threads or processes - result of PeekNamedPipe for partial reads - tests for very large messages (which don't fit in the system buffer) - zero byte messages
In addition to the problems above: The current tests are in a very bad shape. What is the purpose of something like that below? It just hides test failures and regressions ... --- snip --- if (avail != sizeof(obuf)) /* older Linux kernels only return the first write here */ ok(avail == sizeof(obuf) + sizeof(obuf2), "peek4 got %d bytes available\n", avail); --- /snip ---
I wrote all the remaining tests during the last few days, and afterwards we evaluated all possible strategies. The best one we found (for Linux) is using a specific feature called SO_PEEK_OFF, which is only available for kernel >= 3.4, and allows to implement everything "almost" correct. So far we don't have a good solution for MacOS yet, but probably there is something similar - I don't have a Mac, so I cannot really test unfortunately.
You can find my proof-of-concept patch here (it also contains parts by Erich E. Hoover and one test by Adam Martinson): --> http://ix.io/dKs
Splitted version (requires ./tools/make_requests): --> https://github.com/compholio/wine-compholio/tree/namedpipe/patches/kernel32-...
After I've found out how to deal with the ugly existing tests (remove such ifs?!), I'll start submitting the tests, one after each other. This might take some more time, so please be patient ... ;) The actual implementation will follow afterwards, and might take even some more time. In the meantime please test my patches above, and report if it works for your app. They should apply cleanly on wine-1.7.23-33-gc654b7b.
Regards, Sebastian
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #155 from Austin English austinenglish@gmail.com --- (In reply to Sebastian Lackner from comment #154)
After I've found out how to deal with the ugly existing tests (remove such ifs?!), I'll start submitting the tests, one after each other. This might take some more time, so please be patient ... ;) The actual implementation will follow afterwards, and might take even some more time. In the meantime please test my patches above, and report if it works for your app. They should apply cleanly on wine-1.7.23-33-gc654b7b.
Hi Sebastian,
Thanks for tackling this. I tested the patchset against a few of the applications reported in bug 17273, and put the results in a google doc here: https://docs.google.com/spreadsheets/d/1-xOFXZDqAxzhVxcKO3YxHybxr0NF984CgZWJ...
(should be publicly viewable, let me know if it's not). The patchet worked for a couple applications (Calibre, Football Manager 2014 Demo), but fails for some other applications (Windows 8 SDK, Visual Studio 2012, PDFXChange Editor 3.0).
I also tested with the Python 3.1 multiprocessing test, which crashes without the patch. With the patch, that crash disappears, but later tests fail (I haven't looked at what those tests are doing, it could be a different failure or testing NamedPipes in more depth).
Since a few major applications are broken, I haven't tested the others yet, I'll wait for the next revision ;).
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #156 from Sebastian Lackner sebastian@fds-team.de --- Thanks for testing. :) I would guess the remaining failures are caused by either:
* the CloseHandle tests which are still todo_wine so far, or * the fact that on linux we have a message size limit smaller than on Windows. This can be modified by using /proc.
I'm currently updating the patches, and will add a FIXME for the message size - this should allow manually adjusting (or at least notice when something goes wrong). I'll also check if I can do some testing myself for those apps, which still don't pass the test.
http://bugs.winehq.org/show_bug.cgi?id=17195
dmitrii.pukhkaiev@yandex.ua changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dmitrii.pukhkaiev@yandex.ua
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #157 from Sebastian Lackner sebastian@fds-team.de --- Time for more testing! :) I have just updated the patches:
https://github.com/compholio/wine-compholio/tree/namedpipe/patches/kernel32-... (requires ./tools/make_requests)
The main remaining problem with the installers was that they try to change the named pipe handle state before a client has connected (which fails, because only a pseudo_fd has been assigned at that point). The updated patchset stores the state in the server until the socket is finally created. Besides that I've also cleaned up and reordered some parts, and have added support for nonblocking named pipe modes.
I've tested with PDFXChange Editor and the Windows 8 SDK so far. In both cases the patches let it start properly, but in order to finish the installation successfully, you'll also need Erichs ACL patches:
https://github.com/compholio/wine-compholio/tree/master/patches/server-Store... https://github.com/compholio/wine-compholio/tree/master/patches/server-Inher... https://github.com/compholio/wine-compholio/tree/master/patches/server-Misc_... https://github.com/compholio/wine-compholio/tree/master/patches/shell32-Defa... (requires ./tools/make_requests and autoreconf, run ./configure with --with-xattr)
For those that want to test with _all_ the patches I mentioned above - here is a merged patchfile: http://ix.io/dQC
Regards, Sebastian
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #158 from Berillions berillions@gmail.com --- Hi Sebastian,
With your patches, the vcrun 2012 setup finnish successfully. I must to try with my game "Warlock 2 : The Exiled" which needs this application to run correctly.
http://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #159 from Sebastian Lackner sebastian@fds-team.de --- Concerning "Tu Go": The patches also seem to improve it, but you have to set the Wineprefix to Windows 7. If you don't do that you'll see no Window, and just get a message in the logs: "Condition 'VersionNT >= v6.1' evaluates to false.". Unfortunately, even with the Wineprefix set to Windows 7, it still crashes later, during the installation. I suspect that this is some kind of error with Mono.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #160 from Sebastian Lackner sebastian@fds-team.de --- I decided to check what the remaining differences are between my implementation and Adams patchset. I've rebased most of his patches for 1.7.14, and have just added my tests as a last patch (you can split it with 'git am'): http://ix.io/dT6
As suspected, there are various differences in the implementation, test results are here: http://ix.io/dT5 .
Failing tests in my implementation (and marked as todo_wine): * Its impossible to handle specific 0 byte reads correctly (without kernel patches), Adams implementation works well for those. * Based on the design decision to use SOCK_SEQPACKET there is an upper message size limit (which can be changed in /proc). * Some overlapped stuff needs to be fixed, my patches don't touch this part.
Failing tests in Adams implementation: * Writing to a pipe doesn't seem to block if the pipe "is full" - not sure if there are any applications that really needs this though. * Wrong error code when pipe is empty in non-blocking mode, should be ERROR_NO_DATA, but is ERROR_NO_MORE_ITEMS, could maybe confuse some applications? * Doesn't fix all GetNamedPipeHandleState issues. * ReadFile succeeds reading from INVALID_HANDLE_VALUE? Could also confuse some applications, when they depend the correct error code.
This is my attempt to quickly fix the issues 2-4 in his implementation by just adding one additional patch on top of it: http://ix.io/dTc
Regards, Sebastian
https://bugs.winehq.org/show_bug.cgi?id=17195
Martin Baute solar@rootdirectory.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |solar@rootdirectory.de
https://bugs.winehq.org/show_bug.cgi?id=17195
Krzysztof Dębski fantom15@poczta.onet.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fantom15@poczta.onet.pl
--- Comment #161 from Krzysztof Dębski fantom15@poczta.onet.pl --- I think that SOCK_SEQPACKET is supported on FreeBSD (http://lists.freebsd.org/pipermail/svn-src-head/2009-October/010832.html) but the maximum size of a message can be a problem:
$ cat /proc/sys/net/core/wmem_max 163840
We may just ask users to raise the limit if some of their appliactions fail.
Have you checked your patch on an older version on Linux (without SO_PEEK_OFF)?
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #162 from Sebastian Lackner sebastian@fds-team.de --- When SO_PEEK_OFF is not supported, then my implementation will fall back to SOCK_STREAM sockets. This is also the behaviour for kernels < 3.4. Without having SO_PEEK_OFF its not possible to receive parts of a message, so it will not be used at all, to prevent regressions. In this case the patches will only fix the SetNamedPipeHandleState issue, but not all the other details (and will print fixmes). For FreeBSD we would need a similar functionality. If someone experienced with the FreeBSD kernel has some time, feel free to join #wine-compholio (on freenode) to discuss the details about possible implementations. We need either a completely different solution (if available), or will have to reimplement SO_PEEK_OFF there.
BTW, short status update: The first four relatively small Wine patches (which should be definitely correct) are currently waiting to be reviewed. Besides that I'm just updating some later patches in the series, some additional tests revealed that overlapped stuff doesn't work correctly yet, with both Adams and my patchset. Moreover we're preparing some kernel patches, we discovered some weird behaviour for zero byte reads, which can be considered a kernel bug... ;)
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #163 from Adam Martinson adam.r.martinson@gmail.com --- (In reply to Sebastian Lackner from comment #162)
Moreover we're preparing some kernel patches...
Nice to see somebody working on this again =) A few notes from my time on this:
If you need to patch the native kernel to get your implementation to work, it's probably the wrong way to go. As you noted I played around with a SOCK_SEQPACKET implementation, and ditched it because of some of the problems you've run into.
The reason I implemented message mode through the server is because that's what AJ suggested. I think that's probably the easiest & most straightforward way to do it.
If you're really set on using native pipes/sockets, I would take a look at encoding the data in a normal stream socket. Basically save yourself an end-of-message bit out of each byte, and shift the rest of the data accordingly. You'll end up with an overhead of 1 byte per 7 sent, which should give you acceptable performance. Zero-length writes would become a single byte with just that end-of-message bit set. You'd obviously need to translate lengths, which should be trivial.
Any questions, etc feel free to send me an email. ~Adam
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #164 from Luke Kenneth Casson Leighton lkcl@lkcl.net --- (In reply to Adam Martinson from comment #163)
(In reply to Sebastian Lackner from comment #162)
The reason I implemented message mode through the server is because that's what AJ suggested. I think that's probably the easiest & most straightforward way to do it.
If you're really set on using native pipes/sockets, I would take a look at encoding the data in a normal stream socket.
unfortunately, that is the implementation that cannot work, because you can have multiple readers of the socket, none of which may coordinate properly about which may read data and which may read OOB data, which is the whole reason why i created the "one socket pair per message" prototype in the first place.
if you think "ok then, if that's not possible let's do it in the server" then that *also* is not possible because the server must never block and unfortunately you need blocking in order to create the blocking semantics needed by the clients... and you may *not* block in the server.
this is why i said - right back after i stopped working on this - that it is necessary to make wineserver a multi-threaded server because you can allocate one thread per blocking-read.
and the reason why i stopped is because AJ was such a complete dick, giving blanket "fuck off" answers instead of listening to sound logical reasoning.
which is why he should be fired from the project, if he cannot listen to simple logical reasoning, no matter how much redesign work is needed.
AJ if you cannot listen to simple logical reasoning, even if it means that a lot of work may be needed, then perhaps it is time for you to quit the wine project.
sorry, all - been round this loop a number of times.
and, to answer your earlier comments, adam: if the project leaders are completely discouraging and disparaging, why should i spend *my* time interacting with them *at all*?
and before you answer that, adam: consider that i am one of the very rare people who tells things as they are. no sugar-coating. sometimes even though people don't like to hear what needs to be done, someone still has to say it.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #165 from Adam Martinson adam.r.martinson@gmail.com --- (In reply to Luke Kenneth Casson Leighton from comment #164)
and, to answer your earlier comments, adam: if the project leaders are completely discouraging and disparaging, why should i spend *my* time interacting with them *at all*?
and before you answer that, adam: consider that i am one of the very rare people who tells things as they are. no sugar-coating. sometimes even though people don't like to hear what needs to be done, someone still has to say it.
Luke: I don't think anyone is suggesting/requesting you spend any time on this. Go away; you're not helping. That's my un-sugar-coated opinion.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #166 from Sebastian Lackner sebastian@fds-team.de --- Hi,
@Adam: The main advantage of using the wineserver is that such a solution is platform independent. Nevertheless your approach had several disadvantages - writing was always nonblocking, it was theoretically possible to flood the wineserver with too many messages, so that no further memory can be allocated, ... besides that it didn't follow the wineserver style convention, and used wrong or no error codes at some places, but thats easy to fix of course.
If we want to use a wineserver solution, then we have to do a lot more work, to implement everything properly (blocking writes, but different partial writes after each other shouldn't result in interleaved output, ...). I personally would prefer having a kernel solution, but at the end its not really my decision. The next step will be to get all my >1000 lines of additional tests upstream, and talking to some kernel developers in the meantime.
Encoding the "end-of-message" flag in the data stream is not possible, since you have no control how many bytes are returned ... unless you do a peek first, but then its subject to race conditions.
@Luke: One socket pair per message also doesn't sound like a good solution, what about weird programs sending one byte after each other? Moreover in byte read mode the messages have to be merged again, which would also lead to race conditions, when NtReadFile only operates only on one socket at a specific time.
I have also no idea why you think its not possible to implement "blocking" behaviour in the wineserver. It is possible of course, and in fact used already in some places, it just works by allocating a wait handle, and then temporarily returning control to the application. While inside a wineserver call no APCs can be handled, so it wouldn't be a good idea anyway, and wouldn't even work in a multithreaded environment. A multithreaded wineserver would be good for performance reasons, but it makes a lot of other things much more difficult, and I doubt that anyone would be able to change all that without introducing new regressions.
And, it was already said multiple times: Please just leave out all your insults and anger, this bug report is not the right place for that. Even if you have a different opinion, you have to accept that Alexandre is the maintainer, and he decides which features are good enough to included in the official version. Noone keeps you from adding experimental features in your own builds.
Regards, Sebastian
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #167 from Luke Kenneth Casson Leighton lkcl@lkcl.net --- (In reply to Adam Martinson from comment #165)
Luke: I don't think anyone is suggesting/requesting you spend any time on this. Go away; you're not helping. That's my un-sugar-coated opinion.
appreciated your honesty. unfortunately i have a duty and responsibility towards the wider software libre community that extends beyond way beyond what people think of me.
so, apologies, but i am duty-bound to raise the issues that individual communities (and individual leaders) do not wish to discuss.
within each software libre project there is a general belief that an individual leader is "God", and that their word is absolute law.
however within the wider context those "Gods" end up with very narrow focusses that keep projects from expanding.
so, sorry adam, but i am not going away.
l.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #168 from Austin English austinenglish@gmail.com --- (In reply to Luke Kenneth Casson Leighton from comment #167)
(In reply to Adam Martinson from comment #165)
Luke: I don't think anyone is suggesting/requesting you spend any time on this. Go away; you're not helping. That's my un-sugar-coated opinion.
appreciated your honesty. unfortunately i have a duty and responsibility towards the wider software libre community that extends beyond way beyond what people think of me.
so, apologies, but i am duty-bound to raise the issues that individual communities (and individual leaders) do not wish to discuss.
within each software libre project there is a general belief that an individual leader is "God", and that their word is absolute law.
however within the wider context those "Gods" end up with very narrow focusses that keep projects from expanding.
so, sorry adam, but i am not going away.
l.
Discussions and alternative viewpoints are allowed, but outright rudeness and cursing towards developers or users is not appropriate behavior on winehq.org.
I'm happy to enforce that via bugzilla bans, if necessary.
https://bugs.winehq.org/show_bug.cgi?id=17195
Jay Faulkner jayleefaulkner@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jayleefaulkner@gmail.com
https://bugs.winehq.org/show_bug.cgi?id=17195
silva_daniel25@hotmail.com silva_daniel25@hotmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |silva_daniel25@hotmail.com
https://bugs.winehq.org/show_bug.cgi?id=17195
Anastasius Focht focht@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |imwellcushtymelike@gmail.co | |m
--- Comment #169 from Anastasius Focht focht@gmx.net --- *** Bug 22680 has been marked as a duplicate of this bug. ***
https://bugs.winehq.org/show_bug.cgi?id=17195
JKAbrams j@jkabrams.se changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |j@jkabrams.se
https://bugs.winehq.org/show_bug.cgi?id=17195
Anastasius Focht focht@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |1.1.14 Summary|NamedPipe datagrams need to |Multiple applications and |be _really_ datagrams |games need support for | |named pipe message mode | |(NamedPipe datagrams need | |to be _really_ datagrams)
--- Comment #170 from Anastasius Focht focht@gmx.net --- Hello folks,
since this is the only (meta)bug left I'm refining the summary a bit while retaining the original text to better target dupes here.
Feel free to reference the bug in appdb entries as needed.
I've successfully tested Adobe CS5.x product installers with the current patchset from 'Wine-staging' repository, 'namedpipe' branch.
https://bugs.winehq.org/show_bug.cgi?id=22680#c20
Regards
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #171 from Andrey andrey.gursky@e-mail.ua --- (In reply to Anastasius Focht)
I cherry picked/rebased some patches from Wine-Staging (formerly Wine-Compholio) 'namedpipe' branch: https://github.com/wine-compholio/wine-staging/tree/namedpipe/patches/kernel... to Wine 1.7.31+ and it made the CS5 installer succeed in clean 32-bit WINEPREFIX.
Hi Anastasius,
do you maintain these rebased patches on some public VCS server?
(In reply to Sebastian Lackner)
Hi Sebastian,
do you plan to update namedpipe branch?
Regards, Andrey
https://bugs.winehq.org/show_bug.cgi?id=17195
Anastasius Focht focht@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |marius.andreiana@gmail.com
--- Comment #172 from Anastasius Focht focht@gmx.net --- *** Bug 25955 has been marked as a duplicate of this bug. ***
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #173 from Sebastian Lackner sebastian@fds-team.de --- (In reply to Andrey from comment #171)
(In reply to Anastasius Focht)
I cherry picked/rebased some patches from Wine-Staging (formerly Wine-Compholio) 'namedpipe' branch: https://github.com/wine-compholio/wine-staging/tree/namedpipe/patches/kernel... to Wine 1.7.31+ and it made the CS5 installer succeed in clean 32-bit WINEPREFIX.
Hi Anastasius,
do you maintain these rebased patches on some public VCS server?
(In reply to Sebastian Lackner)
Hi Sebastian,
do you plan to update namedpipe branch?
Regards, Andrey
Sorry, I didn't see your reply earlier, my mail program thought it was spam somehow O_o.
I've just rebased the patches and added them to the main branch - this means they'll now be rebased (like the rest of our staging patchset) with every new upstream change:
https://github.com/wine-compholio/wine-staging/tree/master/patches/kernel32-...
Unfortunately FreeBSD is still not supported, but it should at least compile now (have added some additional #ifdefs). If some experienced FreeBSD guy has any idea how to provide the same functionality, feel free to talk to us in #wine-staging.
https://bugs.winehq.org/show_bug.cgi?id=17195
Linards linards.liepins@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |linards.liepins@gmail.com
--- Comment #174 from Linards linards.liepins@gmail.com --- Wow ... there was a lot of heat in the 2012' :)
Anywhow - whats the status? Did the patches land in 1.7.31+ in upstream ?
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #175 from Luke Kenneth Casson Leighton lkcl@lkcl.net --- (In reply to Linards from comment #174)
Wow ... there was a lot of heat in the 2012' :)
yep - there's a long-standing history of people getting unnecessarily angry in the software libre community, and wine is no exception.
i'm able to think things through logically several steps ahead: for some reason people in the software libre world don't like that. i think probably because too far ahead they get lost, ignore the intermediate logical reasoning chain, and then think i'm "ordering" them what to do because in their mind there's no possible supporting evidence for the conclusion that's been reached.
best thing you can do linards is let them get on with it, and learn from their mistakes.
Anywhow - whats the status? Did the patches land in 1.7.31+ in upstream ?
if they're platform-specific and expecting other platforms such as freebsd to add kernel support specifically, solely and exclusively so that the current proposed patch will work, then i certainly hope absolutely not. additionally, relying on a very very specific and obscure and not very well documented implementation of TCP that happens to be present in *current* versions of the linux kernel would be extremely fragile for something as important as this feature.
basically linards to solve this properly and in a multi-platform way that's reliable, wineserver needs to go multi-threaded. the "waiting" that's needed for each named pipe can then be implemented by a thread, blocking on an internal socket exclusive to that thread. you can't let wineserver block (EVER. it would be fatal), and you can't use any other posix socket tricks because posix *specifically* doesn't have anything like NamedPipes.
there does exist a shared memory api which has very similar characteristics to NamedPipes, but unfortunately most posix implementations are not very good (because nobody really uses them). for example, all shared memory handles are global, but once they're created you can't even globally list them to find out what was created let alone find out _what_ created it! which is insane, as the only way to clean up a system of old shm file handles would be to reboot it.
now, when i mentioned that to solve this properly in a multi-platform way, wineserver would need to go multithreaded, alexandre (the paid project leader) went absolutely ballistic. he's been forcing people to explore other options, ever since.
i realise that making wineserver multithreaded is a huge step. however it's one that really should have been done right from the start. the NT kernel is pre-emptive and multi-threaded, and making wineserver *not* emulate that has some unintended side-effects that result in very obscure performance degradation in certain multi-threaded userspace w32/w64 applications. but, as a first implementation, i see no reason why the core infrastructure should not be added, but then *all* functions (except NamedPipes) made non-reentrant (as if wineserver were a single process).
so the problem, linards, is that this is a complex issue of endeavouring to implement a kernel-level feature of one OS (NT) in *userspace* of POSIX OSes (GNU/Linux, FreeBSD, Solaris) in a multi-platform portable and *reliable* way. those requirements leave you with a total of *one* way discovered and proposed so far... and that one way has been flat-out rejected by the lead developer with absolutely no discussion whatsover.
the only reason why i have continued the discussion this far is because i have absolutely no emotional or other invested interest in the proceedings. i see something, i speak up. i won't say that i don't care what people think of me if i speak up: if i happen to care what people think of me it *genuinely* takes *second place* to speaking my mind.
the rest of the people involved however have some form of vested interest in ensuring their long-term involvement with this project, and that means that they have to keep their heads down. if the leader (alexandre juillard) dictates "no, this will not happen", then for the sake of wishing to remain involved with the project, they bow to his will, regardless of the consequences.
this isn't healthy - for them, for alexandre, or for the wine project itself, but that's what they've chosen to do. you or i can provide some external insight informing everyone that that's what's happening, but ultimately linards they have to sort it out for themselves.
https://bugs.winehq.org/show_bug.cgi?id=17195
Michael Müller michael@fds-team.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |STAGED CC| |michael@fds-team.de Staged patchset| |https://github.com/wine-com | |pholio/wine-staging/tree/ma | |ster/patches/kernel32-Named | |_Pipe
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #176 from Luke Kenneth Casson Leighton lkcl@lkcl.net --- http://tech.slashdot.org/story/15/10/04/216221/openindiana-hipster-201510-ke...
so here is an example of a libre OS that is still available, and is still being maintained (despite the difficulty of doing so).
it is not "theoretical" that there are other OSes other than linux-based ones, it is *real* that there are other OSes other than linux-based ones.
i trust therefore that this patch - now staged - is portable across multiple POSIX compliant OSes.
https://bugs.winehq.org/show_bug.cgi?id=17195
Jeff Muizelaar jrmuizel@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jrmuizel@gmail.com
https://bugs.winehq.org/show_bug.cgi?id=17195
Robert Walker bob.mt.wya@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |bob.mt.wya@gmail.com
https://bugs.winehq.org/show_bug.cgi?id=17195
Jacek Caban jacek@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jacek@codeweavers.com
--- Comment #177 from Jacek Caban jacek@codeweavers.com --- Since commit b682e1c41d89c9303d30312c83a402d0cdee4ece, we have support for message more writes (NAMED_PIPE_MESSAGE_STREAM_WRITE). A patch for reads (NAMED_PIPE_MESSAGE_STREAM_READ) is on its way:
https://source.winehq.org/patches/data/131854
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #178 from Luke Kenneth Casson Leighton lkcl@lkcl.net --- (In reply to Jacek Caban from comment #177)
Since commit b682e1c41d89c9303d30312c83a402d0cdee4ece, we have support for message more writes (NAMED_PIPE_MESSAGE_STREAM_WRITE). A patch for reads (NAMED_PIPE_MESSAGE_STREAM_READ) is on its way:
hi jacek,
ok first question: i assume that both patches have had some form of review and discussion? if so, where is that taking place so that the (increasingly large - 54 at the present time after 9 years) list of people interested in seeing this be fixed can get involved, make comments and so on?
second, from looking at the patch i have an immediate question: i assume that you have some code outstanding: have you run the tests that are attached to this bugreport in order to confirm that the known race-conditions outlined in the majority of proposed implementations are catered for?
specifically: when multiple threads are set up to read and multiple threads are set up to write to the exact same pipe, the windows w32 API has "locking" around the length-data tuple which absolutely guarantees that only one reader will have access to the actual pipe. otherwise, what happens is: one reader reads a little bit from the pipe thinking it's "data", and it happens to *INCLUDE* the length!
likewise when there are multiple writers, you get an overlap of length1-length2-data1-data2 or worse length1-part-of-data1 length2-part-of-data2-part-of-data1-part-of-data2
can i suggest reading *carefully* (ignoring the various people who keep thinking i'm being a fucking idiot), the whole of this bugreport as it really is extremely complex.
basically the only solutions that will actually work in a cross-platform way are: (1) shared memory (which has been ruled out in an extremely rude way by julian but despite that i firmly agree with him: globally shared memory can't be recovered / reopened if an app closes) or (2) wineserver has to go multi-threaded in order to handle the "blocking" semantics. unfortunately, here *yet again* julian has (in an extremely deeply unimpressively rude way) flat-out said "no" with absolutely no explanation as to why.
and that's really all that you need to know about why this bug is now eight years old, and despite being Priority 2 and is the cause of several critical programs failing, still hasn't been fixed, because the *actual* problem is nothing to do with programming or source code: the *actual* problem is a systemic failure of wine's interaction with "outsiders". there was a report on slashdot well over 10 years ago about this and it still hasn't really been resolved.
ah well.
anyway if you're brave enough to face up to that, then you'll do okay, and i'll be honoured to help advise on a proposal that, in the many years since this was raised, i believe may actually work in a cross-platform way which might actually be acceptable (and, later, could be adapted to improve wine and fix some of the extremely obscure performance bugs).
as always: feel free to ignore my input and advice if you so choose, but bear in mind that in doing so you directly take all responsibility for the consequences. this is *very* low-level and great care has to be taken to ensure that it really, really is technically correct.
also, you may be aware that once it *is* fixed, a whole slew of other errors will crop up, not because this bug *isn't* implemented correctly but because it *IS* implemented correctly.
if you implement it wrongly it is the worst of all possible worlds... so be really *really* sure ok!
https://bugs.winehq.org/show_bug.cgi?id=17195
winetest@luukku.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |winetest@luukku.com
--- Comment #179 from winetest@luukku.com --- Well wine-staging has includded a series of patches related to this who knows how long alredy. So it's actually fixed in staging, but the patches arent merged in upstream. Can't say about the correctness of the patches. But I have pretty much deep trust in Slackner's work.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #180 from Saulius K. saulius2@gmail.com --- (In reply to winetest from comment #179)
So it's actually fixed in staging, but the patches arent merged in upstream. > Can't say about the correctness of the patches. But I have pretty much deep trust in Slackner's work.
Quick grep show that last patches from Sebastian, Michael and Adam were commited only a year ago. Jacek seems to be working on this since 2016-10: https://source.winehq.org/git/wine.git/?a=search&st=commit&s=pipe%5B...
And I see no Sign-offs or Froms mentioning Sebastian (and Co.:) here, so an update on current situation / work being carried out would be very interesting to get (from Sebastian, Jacek or esp. Alexandre).
(It was a while since I hanged on reading wine-devel@ daily – I miss these times so much:)
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #181 from Luke Kenneth Casson Leighton lkcl@lkcl.net --- (In reply to winetest from comment #179)
Well wine-staging has includded a series of patches related to this who knows how long alredy. So it's actually fixed in staging,
ah.... from what i remember, the patches that adam wrote are based on a very special, unique and specific feature of the linux kernel's TCP/IP stack. in other words, they're completely non-portable. meaning: FreeBSD and other OSes which did not have that specific feature.
however... reviewing what he wrote very quickly i'm not seeing any evidence of the use of TCP/IP so that could have just been an idea that (fortunately!) wasn't used.
but the patches arent merged in upstream. Can't say about the correctness of the patches.
from a quick scan i'm seeing that there's only been one extra test added, which does *not* include the absolutely critical multi-thread-reader and multi-thread-writer tests that i created which demonstrated the correctness of the (excruciatingly-slow-and-brain-dead) prototype patches many years ago.
i cannot emphasise enough how important it is to double-check that the race conditions of multi-threaded readers, multi-threaded writers *and* multithreaded simultaneous readers-and-writers all pass.
this really is an incredibly obscure and deceptively complex scenario which is why it's been left for such a long time. i understood it well at the time but couldn't possibly describe it now without spending significant time on it.
But I have pretty much deep trust in Slackner's work.
that's a very good sign. i do have to say though that this particular scenario is about as complex as the threading library implementation that had to go into wine (which is identical to that of the freedce threading implementation which i'm also familiar with), and it has _that_ much of a low-level impact, not least because it's part of the MSRPC infrastructure (NamedPipes *are* the means by which MSRPC communicates).
so if you get this wrong you *can't even start up wine* which is as good a test as any... *but*... because wineserver is single-threaded it's very very deceptive (i.e. insufficient). i recall suggesting adding (and using) a separate implementation of ncalrpc as a means to bypass this limitation, so that at least the WINREG and other critical infrastructure on which the tests rely can get up and running... but anyway.
so, trust is great... but i would strongly suggest adding the required tests *before* proceeding further. that gives an objective benchmark which everyone can double-check, very easily.
https://bugs.winehq.org/show_bug.cgi?id=17195
nE0sIghT update.microsoft@mail.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- CC|update.microsoft@mail.ru |
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #182 from Jacek Caban jacek@codeweavers.com ---
And I see no Sign-offs or Froms mentioning Sebastian (and Co.:) here, so an update on current situation / work being carried out would be very interesting to get (from Sebastian, Jacek or esp. Alexandre).
Sure, here is an overview of the sutuation. If you're interested in patch reviews, see wine-devel. All my commits to server (and some to ntdll) since October were related.
I revisited what was already discussed in the bug: implement named pipes entirely in wineserver. The implementation uses wineserver-based I/O implemented by Alexandre in a generic way when he was implementing implementing device-based I/O. What we have in the tree since last Friday is named pipe implementation inside wineserver, where we can control everything, including data flow and blocking. The committed series of patches contained all features that previous Wine client-side implementation had, but I tried to move as much as possible out of the series to limit the scope of a single very risky commit. So app-visible fixes we have already are:
- zero length message reads - peek bound to message size - buffer handling and blocking identical to Windows - proper behaviour on pipe end close
That should cover compatibility with applications that use NAMED_PIPE_MESSAGE_STREAM_WRITE flag (without NAMED_PIPE_MESSAGE_STREAM_READ). I believe that this bug is mostly about NAMED_PIPE_MESSAGE_STREAM_READ, which is easy to fix now, but one more patch is still needed: https://source.winehq.org/patches/data/131854
wine-staging has a different and nicely working message mode implementation. The way it's implemented by Sebastian has its advantages. Most notably, its performance is very good. Unfortunately, it has also a problem of being not portable. That was actually my direct reason to revisit the bug: I needed something working on macOS, so I couldn't use Sebastian's patches. The result of my work is portable across all platforms that Wine supports. The interesting thing is that both implementations don't really conflict with each other. wine-staging implementation is mostly on client side and is very much like what we still use for byte mode, so Sebastian may rebase it if he wishes.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #183 from Luke Kenneth Casson Leighton lkcl@lkcl.net --- something's been puzzling me for several hours, now. i reviewed the conversation so far, looking at what i wrote back in 2009, and the very useful comments and input from juan as i first developed the tests, then a first prototype, then extended the tests.
then adam came in, and we discussed the tests, and it's clear that he fully understood the importance of the multi-read and multi-write tests.
alexandre then butts in with some incredibly rude and dismissive comments, and adam is then encouraged to *privately* talk with alexandre, to "Get How It Should Be Implemented As Defined By God Known as Alexandre".
now, what really puzzled me is: why did adam not implement the full set of tests that he *knew* - from discussions with me - were critical to demonstrating full correctness (where running the python 2.7 test suite with the multiprocess module was clearly not enough).
and it occurred to me that if he had done so, he would have had to explain to alexandre why alexandre's over-ruling "I'M RIGHT LKCL IS AN IDIOT DON'T LISTEN TO HIM LISTEN TO ME I'M THE LEADER OF THIS PROJECT" dictats turned out to be wrong.
jacek, i hope that gives you some insight as to what you're up against. you're up not only against an extremely technically-challenging core strategic bottleneck where it's critical to get both high performance *and* characteristics (reliable datagrams) that *do not exist* in POSIX sockets (or they do but they're broken and highly non-portalbe), but you're also dealing with a leader whose attitude towards impartial (uncensored) criticism and constructive feedback that contradicts his "Rule Of Law" is met with what can only best be described as "spoiled brat childish behaviour".
that has, unfortunately, meant that both you and adam and sebastien and myself have all had our time completely wasted, as well as having the consequence that tens of thousands of wine users have been let down... just so that alexandre can prove that he's "in charge".
i'll reply specifically to your message later, after i've read it a couple more times. i've also thought of another couple of tests, one of which is to check if there is write-blocking (due to buffers filling up). if NT has write-blocking on message-mode named pipes, any implementations that don't take that into account are going to result in potentially critical behavioural-changes of applications.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #184 from Austin English austinenglish@gmail.com --- (In reply to Luke Kenneth Casson Leighton from comment #178)
ok first question: i assume that both patches have had some form of review and discussion? if so, where is that taking place so that the (increasingly large - 54 at the present time after 9 years) list of people interested in seeing this be fixed can get involved, make comments and so on?
As with all patches, that discussion is on wine-patches and wine-devel, not bugzilla.
(In reply to Luke Kenneth Casson Leighton from comment #183)
jacek, i hope that gives you some insight as to what you're up against. you're up not only against an extremely technically-challenging core strategic bottleneck where it's critical to get both high performance *and* characteristics (reliable datagrams) that *do not exist* in POSIX sockets (or they do but they're broken and highly non-portalbe), but you're also dealing with a leader whose attitude towards impartial (uncensored) criticism and constructive feedback that contradicts his "Rule Of Law" is met with what can only best be described as "spoiled brat childish behaviour".
Bugzilla is for bugs, not for spreading your views or criticisms of the Wine project. Take such things to your private blog or somewhere else.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #185 from Jacek Caban jacek@codeweavers.com --- I agree with Austin. Luke, for the record, I found Alexandre's input very valuable when I was working on this bug and I appreciate it. The fact that we already have in the tree a working version of what you claimed to be impossible without "multithreaded wineserver" is a prove that he was right on the technical ground.
https://bugs.winehq.org/show_bug.cgi?id=17195
--- Comment #186 from Luke Kenneth Casson Leighton