Thanks for all the detailed comments. But I suppose before going into these details and improving the neats it would be great to hear from Zebediah or Alexandre if this MR is a go in principle. As it is not apparent to me from the initial Zeb's comment.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/384#note_3817
Jinoh Kang (@iamahuman) commented about server/protocol.def:
> - int force_async; /* Force asynchronous mode? */
> + async_data_t async; /* async I/O parameters */
> + int force_async; /* Force asynchronous mode? */
> @REPLY
> obj_handle_t wait; /* handle to wait on for blocking send */
> unsigned int options; /* device open options */
> int nonblocking; /* is socket non-blocking? */
> + int fixup_type; /* socket protocol fixup */
> +@END
> +
> +
> +/* Store info needed for protocol fixup */
> +@REQ(socket_fixup_send_data)
> + obj_handle_t handle; /* socket handle */
> + unsigned short icmp_id; /* ICMP packet id */
> + unsigned short icmp_seq; /* ICMP packed sequence */
Instead of re-aligning tabbing of `@REQ(send_socket)`, maybe we can also use `int` or `short` here. Although `unsigned short` makes more sense if we're going to restrict the scope of the two new server calls only to ICMP packets.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/384#note_3814
Jinoh Kang (@iamahuman) commented about server/sock.c:
> struct fd *fd;
>
> if (!sock) return;
> +
> + if (sock->fixup_type != req->fixup_type)
> + {
> + /* Protocol fixup information should be reflected in client's async data before the
> + * async is delivered to process. So let the client set up the info and restart the
> + * call. */
> + reply->fixup_type = sock->fixup_type;
> + set_error( STATUS_MORE_PROCESSING_REQUIRED );
> + release_object( sock );
> + return;
> + }
> +
We can save the `fixup_type` round trip in the optimal case by returning `STATUS_MORE_PROCESSING_REQUIRED` _only_ when the async would otherwise enter the pending state (i.e. final `status` is `STATUS_PENDING`), and setting `reply->fixup_type` in all cases (specifically, `status == STATUS_ALERTED`).
Not sure if this improvement would be worth it, though, especially since
1) it doesn't eliminate `STATUS_MORE_PROCESSING_REQUIRED` (the raison d'��tre of which being that can't pass extra info via `APC_ASYNC_IO`, and we can't normally synchronize with the APC either, since it's asynchronous by nature) completely, and thus doesn't help simplify the patch,
2) not many tools rely on ICMP packet performace (save for a few network testing tools), and
3) server calls make us lose in the first place.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/384#note_3812