MicrosoftDocs / feedback

📢 docs.microsoft.com site feedback
https://learn.microsoft.com
Creative Commons Attribution 4.0 International
239 stars 160 forks source link

IcmpEcho2 documentation has multiple issues. #3800

Open bhelyer opened 2 years ago

bhelyer commented 2 years ago

The page in question: https://docs.microsoft.com/en-us/windows/win32/api/icmpapi/nf-icmpapi-icmpsendecho2

Apologies for the vague title, but I couldn't condense all my thoughts into something pithier.

I'll start with the reply buffer size documentation:

The allocated size, in bytes, of the reply buffer. The buffer should be large enough to hold at least one ICMP_ECHO_REPLY structure plus RequestSize bytes of data.

This buffer should also be large enough to also hold 8 more bytes of data (the size of an ICMP error message) plus space for an IO_STATUS_BLOCK structure.

Why is the size awkwardly split across multiple paragraphs? I think expressing the total size required in one sentence: "The buffer should be large enough to hold at least one ICMP_ECHO_REPLY, an IO_STATUS_BLOCK, the entire Request buffer, and an additional 8 bytes." would make misreadings less likely. It's complicated, but the fact that that the second half of the size is split across not just sentences but paragraphs tripped me up, and I expect others too (look for async calls to IcmpEcho2 on the internet -- everyone seems to pass different values!)

Further more, IO_STATUS_BLOCK (which appears to be a kernel struct) is not used in the example, which uses sizeof (ICMP_ECHO_REPLY) + sizeof (SendData) + 8 to size the buffer. Is the example or the documentation wrong?

Next up, the return value:

When called asynchronously, the IcmpSendEcho2 function returns ERROR_IO_PENDING to indicate the operation is in progress.

This doesn't seem to be the case here on Windows 10, or any code I've seen using IcmpEcho2 async on the internet. It apparently always returns 0, and GetLastError returns ERROR_IO_PENDING. Maybe this is different when using the ApcRoutine parameter. Maybe it's different in different versions of Windows?

Next, the remarks section.

The application must parse the data pointed to by ReplyBuffer parameter using the IcmpParseReplies function.

If I'm reading this right, when IcmpEcho2 is used async you have to call a different function and then cast the pointer as a different type (PICMP_ECHO_REPLY32). Those two pieces of information seem important enough to move up, rather than being buried in the remarks section and on another function's documentation page respectively -- maybe underneath the ReplyBuffer parameter section?


A small tangent on IcmpParseReplies:

The brief:

The IcmpParseReplies function parses the reply buffer provided and returns the number of ICMP echo request responses found.

The parameter documentation:

The buffer passed to IcmpSendEcho2. This is rewritten to hold an array of ICMP_ECHO_REPLY structures, its type is PICMP_ECHO_REPLY.

On a 64-bit platform, this buffer is rewritten to hold an array of ICMP_ECHO_REPLY32 structures, its type is PICMP_ECHO_REPLY32.

I wouldn't call rewriting a buffer 'parsing'. The brief should probably mention that it modifies the reply buffer provided, as parsing is usually a read only operation.


Back to SendEcho. The next paragraph (emphasis mine):

If the Event parameter is specified, the IcmpSendEcho2 function is called asynchronously. The event specified in the Event parameter is signaled whenever an ICMP response arrives. Use the CreateEvent function to create this event object.

The fact that the async version signals multiple times may be unexpected (it was to me). I think a reasonable assumption may be that the asynchronous version returns the same data as the synchronous version once the event is signalled. It appears this isn't the case, but such a fundamental behaviour change might be better called out explicitly rather than depending on inferring it from 'whenever' in the remarks section.

As the asynchronous version is so drastically different in semantics to the synchronous version, an example for an asynchronous use would be appreciated.


TL;DR

Apologies for the incoherent brain dump. The concrete issues:

welcome[bot] commented 2 years ago

Thank you for creating the issue! One of our team members will get back to you shortly with additional information. If this is a product issue, please close this and contact the particular product's support instead (see https://support.microsoft.com/allproducts for the list of support websites).

gewarren commented 2 years ago

FYI @drewbatgit

drewbatgit commented 2 years ago

@stevewhims It looks like this header is assigned to you.

stevewhims commented 2 years ago

Thanks for filing this, @bhelyer. I'm investigating.

-Steve

stevewhims commented 1 year ago

Still investigating.

stevewhims commented 1 year ago

UPDATE: Still trying to get help from an SME with this one.

stevewhims commented 1 year ago

UPDATE: I've corrected and simplified the topic. The changes address most of your feedback. I'll keep asking about the details that I believe we haven't yet addressed.

-Steve