[RFC PATCH v4 5/5] server: Replace redundant recv_socket status fields with force_async boolean field.

[Jinoh Kang]

On 2/3/22 08:45, Zebediah Figura wrote:
> On 1/29/22 01:47, Jinoh Kang wrote:
>> diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c
>> index 86f5d0d29a2..4cebb4e7829 100644
>> --- a/dlls/ntdll/unix/socket.c
>> +++ b/dlls/ntdll/unix/socket.c
>> @@ -737,13 +737,9 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
>>           }
>>       }
>>   -    status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY;
>> -    information = 0;
>> -
>>       SERVER_START_REQ( recv_socket )
>>       {
>> -        req->status = status;
>> -        req->total  = information;
>> +        req->force_async = force_async;
>>           req->async  = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) );
>>           req->oob    = !!(unix_flags & MSG_OOB);
>>           status = wine_server_call( req );
>> @@ -753,6 +749,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
>>       }
>>       SERVER_END_REQ;
>>   +    information = 0;
>>       alerted = status == STATUS_ALERTED;
>>       if (alerted)
>>       {
> 
> Do we still need this?

I suppose I can drop it and leave it uninitialised, as long as GCC is happy about it...

> 
>> diff --git a/server/protocol.def b/server/protocol.def
>> index 137e6c5a220..2b870ae22e9 100644
>> --- a/server/protocol.def
>> +++ b/server/protocol.def
>> @@ -1451,8 +1451,7 @@ enum server_fd_type
>>   @REQ(recv_socket)
>>       int          oob;           /* are we receiving OOB data? */
>>       async_data_t async;         /* async I/O parameters */
>> -    unsigned int status;        /* status of initial call */
>> -    unsigned int total;         /* number of bytes already read */
>> +    int          force_async;   /* Force asynchronous mode? */
>>   @REPLY
>>       obj_handle_t wait;          /* handle to wait on for blocking recv */
>>       unsigned int options;       /* device open options */
>> diff --git a/server/sock.c b/server/sock.c
>> index 14d95451420..fbf76870a51 100644
>> --- a/server/sock.c
>> +++ b/server/sock.c
>> @@ -3396,8 +3396,8 @@ struct object *create_socket_device( struct object *root, const struct unicode_s
>>   DECL_HANDLER(recv_socket)
>>   {
>>       struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops );
>> -    unsigned int status = req->status;
>> -    int pending = 0;
>> +    unsigned int status = STATUS_PENDING;
>> +    int pending = req->force_async;
>>       timeout_t timeout = 0;
>>       struct async *async;
>>       struct fd *fd;
>> @@ -3405,8 +3405,8 @@ DECL_HANDLER(recv_socket)
>>       if (!sock) return;
>>       fd = sock->fd;
>>   -    /* recv() returned EWOULDBLOCK, i.e. no data available yet */
>> -    if (status == STATUS_DEVICE_NOT_READY && !sock->nonblocking)
>> +    /* Synchronous, *blocking* I/O requested? */
>> +    if (!req->force_async && !sock->nonblocking)
>>       {
>>           /* Set a timeout on the async if necessary.
>>            *
>> @@ -3417,16 +3417,16 @@ DECL_HANDLER(recv_socket)
>>           if (is_fd_overlapped( fd ))
>>               timeout = (timeout_t)sock->rcvtimeo * -10000;
>>   -        status = STATUS_PENDING;
>> +        pending = 1;
>>       }
>>   -    if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->rd_shutdown)
>> +    if (status == STATUS_PENDING && sock->rd_shutdown)
> 
> I may be misreading difs, but isn't this check for STATUS_PENDING redundant now?

It is.  I just left it so it's easy to reorder those if blocks, now that I think about it, it wouldn't be necessary...

> 
>>           status = STATUS_PIPE_DISCONNECTED;
>>         /* NOTE: If read_q is not empty, we cannot really tell if the already queued asyncs
>>        * NOTE: will not consume all available data; if there's no data available,
>>        * NOTE: the current request won't be immediately satiable. */
>> -    if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &sock->read_q ))
>> +    if (status == STATUS_PENDING && !async_queued( &sock->read_q ))
>>       {
>>           struct pollfd pollfd;
>>           pollfd.fd = get_unix_fd( sock->fd );
> 
> And if so this could probably just become an "else" block.

Ack.

> 
>> @@ -3437,22 +3437,17 @@ DECL_HANDLER(recv_socket)
>>               /* Give the client opportunity to complete synchronously.
>>                * If it turns out that the I/O request is not actually immediately satiable,
>>                * the client may then choose to re-queue the async (with STATUS_PENDING). */
>> -            pending = status == STATUS_PENDING;
>>               status = STATUS_ALERTED;
>>           }
>>       }
>>   +    if (!pending && status == STATUS_PENDING) status = STATUS_DEVICE_NOT_READY;
>> +
> 
> I feel like it's hard to immediately tell what this is doing (e.g. what does "pending" mean?)
> 
> Can this be folded into the conditional that deals with timeouts somehow?

I'll try.  Maybe we can move the rd_shutdown check to the top.

> 
>>       sock->pending_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ);
>>       sock->reported_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ);
>>         if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async )))
>>       {
>> -        if (status == STATUS_SUCCESS)
>> -        {
>> -            struct iosb *iosb = async_get_iosb( async );
>> -            iosb->result = req->total;
>> -            release_object( iosb );
>> -        }
>>           set_error( status );
>>             if (timeout)


-- 
Sincerely,
Jinoh Kang