Re: [PATCH 1/3] ws2_32: Implement a basic synchronous TransmitFile (try 2).

[Sebastian Lackner]

On 08.10.2015 03:11, Erich E. Hoover wrote:
> This patch adds support for the basic file-transfer operation of
> TransmitFile.  It reads the file in as "bytes_per_send" chunks, which
> it then sends to the destination using WS2_send.  This process is
> repeated synchronously (until patch 5 adds async support) until the
> entire file is sent.
>
> This version has been updated to use a simple APC-safe NtReadFile-like
> routine instead of using NtReadFile.
>
> +    else if (errno != EAGAIN)
> +        status = wsaErrno();

wsaErrno() doesn't return a status.

Nevertheless, the more critical issue / question I see is, if we want to duplicate
ReadFile code into ws2_32. The ntdll version is already very complicated, and your
minimal version might not always be sufficient.

* Missing handling for server_read_file().
* FILE_GetNtStatus() doesn't match sock_get_ntstatus() exactly.
* FD type dependent code is probably simplied a bit too much. The original code
  behaves different depending on the exact FD_TYPE_* at some places.
* NtReadFile will very likely get even more complicated over time, for example when
  a proper solution for message mode pipes is implemented.

[PATCH 1/3] ws2_32: Implement a basic synchronous TransmitFile (try 2).

[Erich E. Hoover]

On Thu, Oct 8, 2015 at 8:11 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote:
> On 08.10.2015 03:11, Erich E. Hoover wrote:
> > This patch adds support for the basic file-transfer operation of
> > TransmitFile.  It reads the file in as "bytes_per_send" chunks, which
> > it then sends to the destination using WS2_send.  This process is
> > repeated synchronously (until patch 5 adds async support) until the
> > entire file is sent.
> >
> > This version has been updated to use a simple APC-safe NtReadFile-like
> > routine instead of using NtReadFile.
> >
> > +    else if (errno != EAGAIN)
> > +        status = wsaErrno();
>
> wsaErrno() doesn't return a status.

Sorry about that, I meant to use sock_get_ntstatus and copied the
wrong function name.
> Nevertheless, the more critical issue / question I see is, if we want to duplicate
> ReadFile code into ws2_32. The ntdll version is already very complicated, and your
> minimal version might not always be sufficient.
>
> * Missing handling for server_read_file().

I don't think we can safely do that in an APC.
> * FILE_GetNtStatus() doesn't match sock_get_ntstatus() exactly.

Is this on purpose or should we be updating one or the other?
> * FD type dependent code is probably simplied a bit too much. The original code
>   behaves different depending on the exact FD_TYPE_* at some places.

My reading of it said that that all had to do with the timeout/looping
behavior.  I'm not sure that we need to care about this since all of
the use cases we have bug reports about are for using TransmitFile on
true files
> * NtReadFile will very likely get even more complicated over time, for example when
>   a proper solution for message mode pipes is implemented.

Ideally we would just use NtReadFile, but that's clearly not possible.
I'm open to other suggestions, but I don't see a way to avoid this.

Best,
Erich

[PATCH 1/3] ws2_32: Implement a basic synchronous TransmitFile (try 2).

[Alexandre Julliard]

"Erich E. Hoover" <erich.e.hoover(a)wine-staging.com> writes:
> My reading of it said that that all had to do with the timeout/looping
> behavior.  I'm not sure that we need to care about this since all of
> the use cases we have bug reports about are for using TransmitFile on
> true files

It's OK to limit it to real files for now, but then you need to check
and fail properly for other file types.

-- 
Alexandre Julliard
julliard(a)winehq.org