Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish incorporating timestamp method selection improvements into Npcap/Libpcap #174

Closed
guyharris opened this issue May 25, 2020 · 5 comments
Closed

Comments

@guyharris
Copy link

@guyharris guyharris commented May 25, 2020

The call might look like

ULONG * PacketGetTimestampModes(LPADAPTER AdapterObject, UINT *NumModes);

which would return a pointer to an allocated array of ULONG values corresponding to supported time stamp modes and set *NumModes to the number of modes in the array. That might require another routine to free that array.

Alternatively, it might look like

int PacketGetTimestampModes(LPADAPTER AdapterObject, ULONG *Modes)

which:

  • if Modes is null, returns either the number of modes on success or -1 on failure;
  • if Modes is non-null, fills in *Modes and returns the number of modes on success or -1 on failure.

The caller would first call it with a null Modes, to find out how big a buffer it needs, and then allocate the buffer and call it again with that buffer. I don't expect that the number of available modes will change over time without, for example, installing a newer version of Npcap, so two calls should suffice.

This might be implemented atop BIOCGNUMTIMESTAMPMODES and BIOCGTIMESTAMPMODES ioctls. The first takes an int-sized buffer and fills it in with a count of the number of time stamp modes; the second takes a pointer to the array of ULONG and fills it in.

This would allow libpcap to implement pcap_list_tstamp_types() with Npcap.

@gbloice
Copy link
Contributor

@gbloice gbloice commented Jul 16, 2020

FWIW, the normal Win32 API idiom for such a function would be something like:

STATUS PacketGetTimestampModes(LPADAPTER AdapterObject, ULONG *Modes, ULONG *cNodes)

which returns an API specific STATUS value, normally typed to an integer, e.g. a LONG with 0 as success, anything else is an error and cNodes indicating the elements available in the Modes array on input and the number of elements written to the array on output. Calling with Modes as a nullptr will set the required size in cNodes.

@guyharris
Copy link
Author

@guyharris guyharris commented Jul 16, 2020

FWIW, the normal Win32 API idiom for such a function would be something like:

STATUS PacketGetTimestampModes(LPADAPTER AdapterObject, ULONG *Modes, ULONG *cNodes)

which returns an API specific STATUS value, normally typed to an integer, e.g. a LONG with 0 as success, anything else is an error and cNodes indicating the elements available in the Modes array on input and the number of elements written to the array on output. Calling with Modes as a nullptr will set the required size in cNodes.

I know of at least one company up in the US state of Washington that has some APIs that don't follow that convention - those APIs return a BOOL to indicate success or failure, with a status being returned by GetLastError() That's how FlushFileBuffers() works, for example. :-)

Other APIs from that company, such as CreateFile() return HANDLEs, with INVALID_HANDLE indicating failure.

The Packet32.dll routines also follow that convention - not that any of them are APIs; I'm one of the few people who should be writing code that calls them. :-) (I.e., the Packet32.dll API is not guaranteed to be stable; it exists primarily to serve libpcap's pcap-npf.c, and if the API changes, so does pcap-npf.c.)

@gbloice
Copy link
Contributor

@gbloice gbloice commented Jul 20, 2020

FWIW, the normal Win32 API idiom for such a function would be something like:

STATUS PacketGetTimestampModes(LPADAPTER AdapterObject, ULONG *Modes, ULONG *cNodes)

which returns an API specific STATUS value, normally typed to an integer, e.g. a LONG with 0 as success, anything else is an error and cNodes indicating the elements available in the Modes array on input and the number of elements written to the array on output. Calling with Modes as a nullptr will set the required size in cNodes.

I know of at least one company up in the US state of Washington that has some APIs that don't follow that convention - those APIs return a BOOL to indicate success or failure, with a status being returned by GetLastError() That's how FlushFileBuffers() works, for example. :-)

Those ones are annoying, hence I chose the "better" example.

Other APIs from that company, such as CreateFile() return HANDLEs, with INVALID_HANDLE indicating failure.

The Packet32.dll routines also follow that convention - not that any of them are APIs; I'm one of the few people who should be writing code that calls them. :-) (I.e., the Packet32.dll API is not guaranteed to be stable; it exists primarily to serve libpcap's pcap-npf.c, and if the API changes, so does pcap-npf.c.)

My observation, which didn't spell it out, was to move the number of nodes from the return to an in\out parameter, which then allows the return to indicate success or failure, or something richer without having to use "out-of-range" values as the return to indicate things went wrong.

@fyodor
Copy link
Member

@fyodor fyodor commented Aug 17, 2020

This is just an update/summary that we added the Packet* calls a while back, but we still need to make and submit a patch to upstream libpcap (and possibly our wpcap.dll while we wait for upstream incorporation). We also should document the Npcap-specific details on how these calls are implemented on Windows into the Npcap users' guide.

@fyodor fyodor changed the title Add a new PacketGetTimestampModes() call, to get a list of time stamp modes supported Finish incorporating timestamp method selection improvements into Npcap/Libpcap Aug 17, 2020
@guyharris
Copy link
Author

@guyharris guyharris commented Aug 17, 2020

This is just an update/summary that we added the Packet* calls a while back, but we still need to make and submit a patch to upstream libpcap

You added PacketSetTimestampMode(), but not PacketGetTimestampModes(); the libpcap changes will require both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants