[PATCH 4/5] server: Added server side mamed pipe read and write implementation and use it for message mode pipes.

Jacek Caban jacek at codeweavers.com
Tue Feb 28 07:03:33 CST 2017


Hi Sebastian,

On 27.02.2017 19:42, Sebastian Lackner wrote:
> On 22.02.2017 14:51, Jacek Caban wrote:
>> Signed-off-by: Jacek Caban <jacek at codeweavers.com>
>> ---
>>  dlls/kernel32/tests/pipe.c |  28 ++-
>>  dlls/ntdll/file.c          |   7 +-
>>  dlls/ntdll/tests/file.c    |   2 +-
>>  server/async.c             |   9 +
>>  server/file.h              |   1 +
>>  server/named_pipe.c        | 446
>> ++++++++++++++++++++++++++++++++++++++++++---
>>  6 files changed, 447 insertions(+), 46 deletions(-)
> As I see this updated patchset allows to return the result immediately,
> but still uses a USR1 signal to transfer back the IO completion info.
> Wouldn't it be useful to get rid of that aswell?

I believe it would be useful, but I assumed it does not block this
patch. It's tricky, but I gave it some thoughts before sending patches
and intend to look at that as some point. Here are some thoughts:

As long as we transfer result over server socket, we don't have control
in server over when iosb and result buffer are filled, so we need a
signal from client when we can signal async completion (signal
event/object, queue completion). I sent a patch that was more inline
with how we do that for calls where I/O is implemented on client side
[1], but Alexandre preferred that to be in server [2]. Having that in
server is not only cleaner, but also gives us more control, like in
which case to fill iosb or signal event. It will also make it possible
to implement other solution (bellow).

As long as we need a way for client to signal that data is transferred,
we need some sort of notification from client. We could add a new server
call, that client would be responsible to call after receiving data,
that we could use instead of wait handle and APCs, but I think that
moving more logic to client is not the right solution.

An other solution is not to transfer result over server socket, but use
write_process_memory() instead. This would allow us to do everything in
a single server call. Unfortunately, it seems to me that there will be
cases (like writing to memory what has write watch), when we will need
APC-based fallback anyway. This is a huge change on its own, it needs at
least:
- Use write_process_memory() in async_terminate() when we know we can do
that for particular async
- Since above will complicate wait handle logic, it would be nice to
move this logic to read/write/ioctl/flush request handlers instead of
duplicating that in fd ops.
- The above will need invasive change for device IRP-based calls, which
currently use irp_call, not async, for wait handles.
- The above has a side quest: it would be nice to consider making device
calls blocking more compatible. I believe that we should always wait
until real driver completes request (even for non-blocking calls) and
return STATUS_PENDING only if driver returned STATUS_PENDING. That's
something to keep in mind while doing design decision, not strictly related.
- Right now, user passed APCs are stored in client and transferred as
APC_ASYNC_IO. This would need to be changed.

I'm sure some of those tasks will need more smaller changes. Those would
be much nicer to work on having named pipes in the tree first, so that
we can at least test the code. And, since I think we will need
APC+blocking as a fallback anyway, which we can use blocking as the main
solution for the time being and have it well tested.

That said, I agree it would be useful, I just think it shouldn't block
the patch.

> Also please note that we need to distinguish "immediate returns" and
> "async returns" to properly implement FileIoCompletionNotificationInformation.
> This especially means that waiting internally is not an option.

It's easy to distinguish and still have internal waits. I can't point
the right solution without looking deeper at
FileIoCompletionNotificationInformation, but just as an example, you
could do:

if (iosb->status != STATUS_PENDING) mark_async_as_immediate_result();

inside request handlers just after calling fd ops. Then you'd have all
the info you need inside async object. Am I missing something?

>> +    iosb = async_get_iosb( async );
>> +    if ((blocking || iosb->status != STATUS_PENDING)
>> +        && !(handle = alloc_handle( current->process, async, SYNCHRONIZE, 0 )))
>> +        async_terminate( async, get_error() );
> I don't think this will work as expected. async_terminate might queue an
> APC, but the caller will immediately deallocate the async struct in server_write_file
> because status != STATUS_PENDING.


Good point, I will send an updated patch.


Thanks for the review,

Jacek


[1] https://www.winehq.org/pipermail/wine-patches/2017-January/157013.html

[2] https://www.winehq.org/pipermail/wine-devel/2017-February/116060.html




More information about the wine-devel mailing list