[patch] review requested - NamedPipe Message mode (was Reliable Datagram / Stream behaviour of NT Pipes)
http://bugs.winehq.org/attachment.cgi?id=19132
ok this is the beginnings of an implementation of messagemode, it passes the test1 and send2recv2 tests and fails the shortread test as part of PeekNamedPipe(). i'm not exactly sure why, yet. i get the distinct feeling that there's confusion over who writes into the wineserver data structures what the "availdata" is. msrpc.exe says "set availdata to 9", msrpcd.exe says "set it to 0, we did a write!" and it's all just ohhhh dear.
also i have double-copies of server_set_named_pipe_info(), one in kernel32 and another in ntdll because i am using it in both places, i think SetNamedPipeHandleState() should be calling an NtFsControlIo rather than using server_set_named_pipe_info() but for the life of me i _really_ don't want to be finding out what the heck that function is :) hmm, maybe i can find out by copying ntdll.dll over to wine and doing a WINEDEBUG=trace+ntdll,kernel32 to see what functions get called.
anyway the primary purpose of submitting the patch is to ask people if i'm along the right lines. anything seriously objectionable, etc. etc.
l.
Luke Kenneth Casson Leighton lkcl@lkcl.net writes:
also i have double-copies of server_set_named_pipe_info(), one in kernel32 and another in ntdll because i am using it in both places, i think SetNamedPipeHandleState() should be calling an NtFsControlIo rather than using server_set_named_pipe_info() but for the life of me i _really_ don't want to be finding out what the heck that function is :) hmm, maybe i can find out by copying ntdll.dll over to wine and doing a WINEDEBUG=trace+ntdll,kernel32 to see what functions get called.
anyway the primary purpose of submitting the patch is to ask people if i'm along the right lines. anything seriously objectionable, etc. etc.
You can't do that stuff on the client side. You either have to do all pipe I/O in the server, or add named pipe support in the kernel. The latter is harder, but would be much more useful.
You can't do that stuff on the client side. You either have to do all pipe I/O in the server, or add named pipe support in the kernel. The latter is harder, but would be much more useful.
well, not entirely knowing the difference, i'm guessing that i'm adding named pipe support in the kernel. starting from SetNamedPipeHandleState and going from there.
comparing what i've got so far in the improvements to ReadFile, WriteFile, NtFsControlIo etc. with the way that it is believed that this _should_ be done, what is right and what is wrong?
l.
Luke Kenneth Casson Leighton lkcl@lkcl.net writes:
You can't do that stuff on the client side. You either have to do all pipe I/O in the server, or add named pipe support in the kernel. The latter is harder, but would be much more useful.
well, not entirely knowing the difference, i'm guessing that i'm adding named pipe support in the kernel. starting from SetNamedPipeHandleState and going from there.
I mean the Linux kernel.
comparing what i've got so far in the improvements to ReadFile, WriteFile, NtFsControlIo etc. with the way that it is believed that this _should_ be done, what is right and what is wrong?
It's all wrong, because this can't be done on the client side (kernel32 and ntdll are the client side, as opposed to the wineserver side). The client side is the equivalent of user space, with the wineserver being the equivalent of kernel space. You can't do this stuff in user space (hint: concurrency).
On Sun, Feb 1, 2009 at 9:29 AM, Alexandre Julliard julliard@winehq.org wrote:
Luke Kenneth Casson Leighton lkcl@lkcl.net writes:
You can't do that stuff on the client side. You either have to do all pipe I/O in the server, or add named pipe support in the kernel. The latter is harder, but would be much more useful.
well, not entirely knowing the difference, i'm guessing that i'm adding named pipe support in the kernel. starting from SetNamedPipeHandleState and going from there.
I mean the Linux kernel.
ohhh, ok. yeah, you're right, it would be _incredibly_ useful to have message-moding / named-pipes in the linux kernel. i do wonder though if the linux developers would actually appreciate its significance - it's "not uuunix", after all. mind you, they _did_ grok the change-notify stuff that was inspired by samba's requirements, so maybe.
... but anyway, i'll leave that one to you [dear wine developers] to deal with, as the main linux kernel developers have no respect for anything i say. it'll have to come from you. i'll help all i can in designing and reviewing such a feature, but the actual proposal - and liasing with the linux kernel developers - will have to come from you.
also, for running wine on non-linux-based (or non-in-kernel-message-mode-supporting unixen) you'd still need "emulation" anyway.
comparing what i've got so far in the improvements to ReadFile, WriteFile, NtFsControlIo etc. with the way that it is believed that this _should_ be done, what is right and what is wrong?
It's all wrong, because this can't be done on the client side (kernel32 and ntdll are the client side, as opposed to the wineserver side).
ok, understood.
The client side is the equivalent of user space, with the wineserver being the equivalent of kernel space. You can't do this stuff in user space (hint: concurrency).
lemme think about that, see if i can write a multi-process or multi-threaded test that makes what i've got here blow up. any help there greatly appreciated. multiple connections to named pipes are ... a bit... odd! the designers forgot to add in the equivalent of listen(), accept() and friends.
question: the existing code just uses server_get_unix_fd() followed by read-and-write on the underlying unix domain socket / unix-pipe (whatever). obviously, to emulate message-mode, a read is now potentially _two_ read operations: one of 4 bytes to get the length-field, and the other read(s) will get the data.
i take it that you're concerned that there will be a race condition where two threads reading from the same handle might do _two_ 4-byte reads, one correct and the other reading "data" as if it was a length-field?
so, adding server_named_pipe_read() avoids this issue by doing locking (like server_get_unix_fd()) does, i see)
well... you see... there could well be a problem with the existing code, on concurrency, because reads and writes can be broken down into unix-specific default buffer-reading/writing sizes (both read and write do a for (;;) { ... read/write } loop
oh - update: http://bugs.winehq.org/attachment.cgi?id=19145
this one passes all three tests.
the rather unfortunate thing is: it has knock-on ramifications for any code *inside* wine that uses NamedPipes, relies on SetNamedPipeHandleState() working correctly!
so, although the tests are passed, the bigger "test" - running wine - it's a bit... mair :) how the heck does wine work _without_ messagemode on named pipes, anyway?? surely there's something keeling over, somewhere.
which brings me on to another quite important question: are there any work-arounds that have been added into rpcrt4 or any other location which says "yes i do messagemode" and then goes "but actually, we know it doesn't work so we do xyz instead"?
btw, i'm dead impressed with the state of the functionality of svcctl, and its idl file. if you've got svcctl working over ncacn_np then that's a _really_ good sign that the MSRPC interoperability is up to scratch (svcctl has those looovely variable-sized extensible structs...) - and a damn good indication that you stand a really good chance of being able to make this "networked", replacing the stub-services in samba (the ones that i kept getting told "have no meaning" to a unix system). in combination with the break-out mechanism in samba, you will _finally_ be able to run "true" nt services.
that'd be absolutely... wild.
you'll still need to use the built-in \pipe\netlogon, \pipe\lsarpc and \pipe\netlogon, for the time being, and if ... oh. i'm sure i've got a netlogon.idl and a samr.idl file somewhere, reverse-engineered. but hey, you can always get the samba ones... _if_ they've put them under an appropriate license. if not, let me know, i'm sure i can find something somewhere.
http://library.pantek.com/Applications/Samba/mirror/unpacked/samba_3_X_test/...
oh. winreg. i see winreg is still a .h file not a .idl file. piityyy :) it'd just be absolute icing on the cake to be able to have /usr/local/bin/wine regedit.exe \remotentboxsomewhere actually _do_ something :)
l.
Luke Kenneth Casson Leighton wrote:
ohhh, ok. yeah, you're right, it would be _incredibly_ useful to have message-moding / named-pipes in the linux kernel.
You may want to check the Longene project (former Linux Unified Kernel). Maybe they already have it implemented in kernel.
so, adding server_named_pipe_read() avoids this issue by doing locking (like server_get_unix_fd()) does, i see)
$ git commit -a -m '#17185 - server-based read_named_pipe. does blocking in client and non-blocking reads (using recv MSG_PEEK) in the server. messy as hell.'
says it all, really :) bit of a marathon session that one. break-time. more in a couple of days. anyone want to take over, email me, you can have what i've got so far. blocking checks client-side (using select) are made on the unix-pipe fd to ensure that there's _actually_ data that wineserver can get at. after reading the message-length (done only once per message duh), read() is replaced by server_named_pipe_read() - still in the for (;;;) loop.
alexandre, would it be reasonable to put a CriticalSection around the [optional message-length-reading and] for-loop? i really can't see any other way. what i've done is protect the server from blocking on a read(), by making sure that recv(MSG_PEEK) - actually select() might be better - checks the length. that means however that you still, in kernel32, in ReadFile(), still have to do the for() loop, but preceding each read by a blocking wait (i clear O_NONBLOCK and do an infinite/blocking select())
it all screams "CriticalSection" protection - on a per-handle basis.
anyone got any better ideas? let me know, i'll come back to this in a couple of days.
l.
-
Alexandre Julliard -
Luke Kenneth Casson Leighton -
Piotr Pawłow