Proposals for v6

[kayoub5]

@chmorgan Those are points that I would like to propose for v6, since it's going to be a major release:

Remove Npcap & WinPcap namespaces

Port all features to Libpcap name space:

• Remote Pcap
• Send Queue
• Statistic Mode Statistics Device #234

Singleton CaptureDeviceList

Currently having a singleton as a CaptureDeviceList causes problems when application needs to create more than one device.
The most common use cases/problems:

• Linux requiring different device instance for sending and receiving.
• Capture Device is not thread safe, making it a singleton causes deadlocks.
• Device does not get closed by the code that opened it.
• Multiple lists exist: CaptureDeviceList , LibPcapLiveDeviceList, thus tracking devices that did not get closed is more difficult

Proposal:

• Make ICaptureDevice implement IDisposable, thus code analysis will automatically report devices that get opened without being closed, capture devices can be wrapped in a using (device) { ... } block.
• Create methods similar in logic to File.Open method:

// PcapInterface is stateless, and the time spent creating it would only be spent once.
var pcapInterface = PcapInterface.GetAllPcapInterfaces()[0];
LibPcapLiveDevice device = Pcap.Open(pcapInterface, [open arguments]);

// From NetworkInterface
// NetworkInterface contains all information needed to create a PcapInterface
var networkInterface = NetworkInterface.GetAllNetworkInterfaces()[0];
LibPcapLiveDevice device = Pcap.Open(pcapInterface, [open arguments]);

// Statistics (
// We could emulate it if not supported natively in Libpcap
StatisticsDevice device = Pcap.OpenStatistics(pcapInterface, [open arguments]);

// File
string path = "path to file";
 CaptureFileReaderDevice device = Pcap.OpenRead(path, [open arguments]);

[chmorgan]

The reason CaptureDeviceList was originally a singleton was due to the time it takes to create one. If we can clean that up and make it simpler for the user I'm all for it.

Was the idea to replicate statistics mode with a wrapper class or did libpcap add support for it?

[kayoub5]

Proposal updated

[kayoub5]

libpcap statistic mode is still windows only.

[kayoub5]

@chmorgan I can't find DeviceMode in libpcap, and it seems OpenFlags supersede it.

Should we remove it/ merge the two enumerations?

[chmorgan]

Yep. It overlaps in functionality.

[kayoub5]

@chmorgan instead of using arguments to Open() method, what do you think about using a DeviceConfiguration class.

[chmorgan]

@kayoub5 as a heads up I'm working on the timestamp precision changes here. Wanted to let you know so we don't overlap with any 6.x changes.

[kayoub5]

@chmorgan Do you have any more planned PRs for timestamp, or I can work on making the PosixTimeval class to make it support nano seconds?

[chmorgan]

@kayoub5 nothing pending at the moment. On the changes to PosixTimeval there may be ways to improve performance by reducing processing there. Each packet requires a new PosixTimeval instance and I'm wondering if its noticeable at high packet rates.

[kayoub5]

@chmorgan with how the API is taking shape, I don't think we need NpcapDeviceList, CaptureDeviceList and LibPcapLiveDeviceList.
Should I keep CaptureDeviceList or LibPcapLiveDeviceList ? or simply remove them all since we have PcapInterface.GetAllPcapInterfaces() ?

[chmorgan]

If its the same api calls provided by ncpap that makes sense. How does the user know if something is npcap device?

[chmorgan]

@kayoub5 thoughts on how we should handle the PosixTimeval with the sec/usec sec/nsec changes? The pcap docs recommend just treating the usec field as nsec but in our case we could have a PosixTimevalNS. I'm not sure that's super helpful for the end user though.

The other thought I had was that we could switch PosixTimeval to always assume nsec and convert from usec to nsec when we read packets but in that case we'd incur a performance penalty for each packet we read and it probably doesn't make sense to do that.

A third approach could be to simply add a nsec field to PosixTimeval and let the user handle which methods/properties they access, basically like the first option above that pcap docs kind of allude to.

I think in any case we can't have anything that impacts performance and it should be easy to figure out for the end user.

Thoughts?

[chmorgan]

@kayoub5 what is the approach to asking a device to configure things it can't? It looks like we have some gaps in our tests today as we don't attempt to test whether invalid options result in errors, for example if you set MinToCopy but that isn't supported we do a configuration error but that defaults to doing nothing.

I'm asking because I'm trying to add a test for timestamp support and in some cases, like old winpcap, this isn't supported. If I go the configuration route I'd have to add a default handler but imo a user should get an exception by default if they try to configure something that isn't supported vs. the current behavior of dumping the exception due to lack of error handler.

Thoughts?

[kayoub5]

The idea proposed in #230 looks good.

[chmorgan]

@kayoub5 thoughts on a v6 release in the next few weeks? Code coverage is up, we've cleaned up a bunch of the api. Anything else you wanted to get in before the first release?

[kayoub5]

I was thinking of making PosixTimeval support nano seconds, since that will most likely require a breaking change.

[chmorgan]

@kayoub5 as long as we aren't slowing the capture down, or slowing it down by any measurable amount, that should be fine. Imo we could just as well add a Nanoseconds property and let the user figure it out on their end based upon the properties of the device. Then they can decide whether to perform some operation on the packets as they are captured (which could slow things down) or save and defer that until offline processing.

[chmorgan]

Alright, it looks like we've slowed down a bit on cleanups and improvements. Might as well push out v6 unless you've got something pending that should go in first. Thoughts?

[kayoub5]

Could you push the new version as beta first?

[chmorgan]

How does that work with nuget versioning? Is there a way to indicate a beta rev?

…

On Sun, Apr 4, 2021 at 5:34 AM Ayoub Kaanich ***@***.***> wrote: Could you push the new version as beta first? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#201 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJH4ADOXA3EE7FLN2HY7Y3THAXEBANCNFSM4WGDELEQ> .

[kayoub5]

you just need to indicate that in the version string
see https://docs.microsoft.com/en-us/nuget/create-packages/prerelease-packages