On 7/12/22 22:29, Paul Gofman (@gofman) wrote:
When initializing the fixup on server I am setting IP_RECVTTL, IP_RECVTOS and IP_PKTINFO socket options. If the app is calling recvmsg() it may get the headers it didn't request. My reasoning that such a situation doesn't deserve further code complication to avoid but deserves a fixme.
Makes sense, thanks.
The async may be initiated on the server and delivered to the process (through SIGUSR1)?? before we set async->fixup_type. It might be not likely in practice, but I double verified that it is possible by injecting Sleep(...) before setting fixup type to async structure from server. So I need to make sure that fixup_type in async structure is correctly set before try_recv() can be reached. So idea is, fixup_type is "no fixup" by default, no extra server call is involved in normal case. But if fixup is required and client does not yet know about it server doesn't?? do anything and just returns fixup type with STATUS_MORE_PROCESSING_REQUIRED.
Right, that is annoying, but I guess it's the most reasonable way to solve that problem.
On the other hand... spitballing, but what if instead of returning anything from the server, we used SO_PROTOCOL and SO_TYPE on the client side to check whether it's a DGRAM + ICMP socket?
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?|
Well, originally I did something between the lines of this (had icmp_over_dgram field in request / reply). But then I looked at that and it felt odd that general recv_socket() has some special icmp related fields, it felt like naming that "fixup_type" tells what it is without going into very specific ICMP fixup details looking weird at this level. If you feel like it is better to name it, e. g., "icmp_over_dgram" I can change it back. I don't currently have any other types of fixups in mind.
On reflection I don't feel strongly about it. I think "is_icmp_dgram" or something is reasonable enough by itself if clarified with a comment, but I can also understand the opposing argument.