On 7/5/22 14:53, Paul Gofman wrote:
@@ -602,9 +623,68 @@ static NTSTATUS try_recv( int fd, struct async_recv_ioctl *async, ULONG_PTR *siz }
status = (hdr.msg_flags & MSG_TRUNC) ? STATUS_BUFFER_OVERFLOW : STATUS_SUCCESS; + if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) + { + unsigned int tot_len = sizeof(struct ip_hdr) + ret; + size_t len = hdr.msg_iov[0].iov_len; + char *buf = hdr.msg_iov[0].iov_base; + struct cmsghdr *cmsg; + struct ip_hdr ip_h; + + if (ret + sizeof(ip_h) > len) + status = STATUS_BUFFER_OVERFLOW; + + if (len < sizeof(ip_h)) + { + ret = len; + } + else + { + ret = min( ret, len - sizeof(ip_h) ); + memmove( buf + sizeof(ip_h), buf, ret ); + ret += sizeof(ip_h); + } + memset( &ip_h, 0, sizeof(ip_h) ); + ip_h.v_hl = (4 << 4) | (sizeof(ip_h) >> 2); + ip_h.tot_len = htons( tot_len ); + ip_h.protocol = 1; + ip_h.saddr = unix_addr.in.sin_addr.s_addr; + + for (cmsg = CMSG_FIRSTHDR( &hdr ); cmsg; cmsg = CMSG_NXTHDR( &hdr, cmsg )) + { + if (cmsg->cmsg_level != IPPROTO_IP) continue; + switch (cmsg->cmsg_type) + { +#if defined(IP_TTL) + case IP_TTL: + ip_h.ttl = *(BYTE *)CMSG_DATA( cmsg ); + break; +#endif +#if defined(IP_TOS) + case IP_TOS: + ip_h.tos = *(BYTE *)CMSG_DATA( cmsg ); + break; +#endif +#if defined(IP_PKTINFO) + case IP_PKTINFO: + { + struct in_pktinfo *info; + + info = (struct in_pktinfo *)CMSG_DATA( cmsg ); + ip_h.daddr = info->ipi_addr.s_addr; + break; + } +#endif + } + } + memcpy( buf, &ip_h, min( sizeof(ip_h), len )); + }
Would you mind making this a helper function? try_recv() is already large and this seems to be pretty self-contained logic.
if (async->control) {
if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM)FIXME( "May return extra control headers.\n" );
What is this for?
@@ -737,17 +818,26 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi } }
- SERVER_START_REQ( recv_socket )
- do {
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 );wait_handle = wine_server_ptr_handle( reply->wait );options = reply->options;nonblocking = reply->nonblocking;- }
- SERVER_END_REQ;
SERVER_START_REQ( recv_socket ){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);req->fixup_type = async->fixup_type;status = wine_server_call( req );if (status == STATUS_MORE_PROCESSING_REQUIRED){async->fixup_type = reply->fixup_type;continue;}wait_handle = wine_server_ptr_handle( reply->wait );options = reply->options;nonblocking = reply->nonblocking;}SERVER_END_REQ;} while (status == STATUS_MORE_PROCESSING_REQUIRED);
alerted = status == STATUS_ALERTED; if (alerted)
I'm missing something; why do we need to call this in a loop? Why does the client needs to communicate the fixup type to the server in the first place?
diff --git a/server/protocol.def b/server/protocol.def index a8beccaf468..e1e5f3d9faa 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1446,17 +1446,23 @@ enum server_fd_type file_pos_t count; /* count of bytes to unlock */ @END
- /* Perform a recv on a socket */ @REQ(recv_socket) int oob; /* are we receiving OOB data? */ async_data_t async; /* async I/O parameters */ int force_async; /* Force asynchronous mode? */
- int fixup_type; /* protocol fixup type returned previously */ @REPLY obj_handle_t wait; /* handle to wait on for blocking recv */ unsigned int options; /* device open options */ int nonblocking; /* is socket non-blocking? */
- int fixup_type; /* protocol fixup type (see below) */ @END
+enum sock_prot_fixup_type +{
- SOCK_PROT_FIXUP_NONE,
- SOCK_PROT_FIXUP_ICMP_OVER_DGRAM,
+};
Making this an enum strikes me as odd in general. A bitmask might make more sense, but frankly I'd just leave it as a dedicated field until we have something else that needs fixups.
Do you have any plans for introducing other types of fixups?