Pcap device list refactoring to use smart pointers. #1386

Dimi1010 · 2024-05-06T12:56:17Z

This PR mainly aims to update PcapLiveDeviceList and PcapRemoteDeviceList to utilize cpp11's unique_ptr for dynamic memory management.

Overview of changes:

Additional minor changes:

Marked PcapLiveDeviceList and PCapRemoteDeviceList's copy ctors as explicitly deleted instead of keeping them private.
Moved internal helper deleters to MemoryUtils.h.
Centralized code for fetching device list from Libpcap/Winpcap/Npcap in DeviceUtils.h/cpp. (It was duplicated a couple of times)

…eviceList with smart pointers. Added a secondary vector of non-owning pointers to the devices to keep the signature of some getter methods as they return references.

… initialization.

…ress, instead of copying the address to a new bytes array.

Dimi1010 · 2024-05-06T13:16:01Z

@seladb On the task list above, regarding PcapRemoteDevice's RemoteAuthentication, should I keep the current functionality by using a shared_ptr or make each device hold a unique copy?

Edit: Decided to go with shared_ptr for now.

…capRemoteDevice to be a shared_ptr.

…ore freedom on the caller, to skip a reference counter increase/decrease by moving.

…optimizations if the caller does not require his own ptr anymore

…aders in MemoryUtils header.

…nagement. Added a second helper 'view' vector to keep raw pointers of the devices for backward compat. Replaced iterator types with auto keyword. Removed destructor as the smart pointers handle cleanup.

… a non-owning raw pointer to allow the device to outlive the list if nessesary.

Dimi1010

@seladb @tigercosmos could you please look at these comments and give an opinion?

Dimi1010 · 2024-05-27T19:35:34Z

Pcap++/header/PcapLiveDeviceList.h

@@ -49,50 +57,105 @@ namespace pcpp

 		/**
 		 * @return A vector containing pointers to all live devices currently installed on the machine
+		 * @deprecated This method is deprecated in favor of the SmartPtrAPI overload.
+		 */
+		PCPP_DEPRECATED_RAW_PTR_API const std::vector<PcapLiveDevice*>& getPcapLiveDevicesList() const;


PcapLiveDeviceList provides a getter API to get all registered devices, while PcapRemoteDeviceList provides a container-like API to get all registered devices.

In my opinion, it would be best if both lists provided a standardized API. Which one would be better?
Here are my opinions on the pros and cons for both:

Getter method:
✔️ A getter approach would be consistent with the rest of the getter methods.
✔️ A getter can be made more easily to have overloads via tag dispatching.
❌ Reference getter locks down the internal implementation to always keeping a member of this type.
❌ Value getter will have a cost every time the getter is invoked, constructing the vector and coping the shared pointers.

Iterator API:
✔️ An iterator API would allow direct integration with other iterator algorithms and for-each statements.
✔️ Low overhead iteration over the devices.
❌ Complications with deprecation of existing iterators.^[1]

[1] As the current PcapRemoteDeviceList already has an iterator API, that would be difficult to change with respect to the deprecation polity of 1 version notice of deprecation. Ideally, the iterators would refer to const std::shared_ptr<T>& with T being the device class. The current implementation returns iterators to *T. Theoretically it is possible to write a converting iterator, but it is going to be complicated. The current stopgap I ended up on is keeping a duplicate list of raw pointers to the devices, which is not ideal.

I think it makes sense to have both a getter and an iterator for a container-like object.

Regarding the iterator type, since std::shared_ptr<T> and T* are pointer types, the users' existing code (such as ->) should not have much impact.

However, if we really don't want to break the API, as you said, it's still possible to have some conversion, but it's just troublesome.

I think it makes sense to have both a getter and an iterator for a container-like object.

Both is an option.

Regarding the iterator type, since std::shared_ptr<T> and T* are pointer types, the users' existing code (such as ->) should not have much impact.

Yeah, they shouldn't really. unless they decided to save the value from the iterator somewhere. That is a breaking change, but it would only need a get() call from the smart pointer to fix, as old code should still keep the list alive to keep the devices alive.

However, if we really don't want to break the API, as you said, it's still possible to have some conversion, but it's just troublesome.

I tried my hand at converting iterator a bit earlier. It is technically possible but it quickly becomes templated black magic if you try to make a general case converting iterator.

I prefer to remove the old API, and have both getter and new iterator. Then we can also remove the hack of SmartPtrApiTag and m_LiveDeviceListView

@seladb what do you say?

Dimi1010 · 2024-05-27T19:36:37Z

Pcap++/header/PcapRemoteDeviceList.h


 		/**
 		 * @return An iterator object pointing to the first PcapRemoteDevice in list
 		 */
-		RemoteDeviceListIterator begin() { return m_RemoteDeviceList.begin(); }
+		RemoteDeviceListIterator begin() { return m_RemoteDeviceListView.begin(); }


See comment in PcapLiveDeviceList on getPcapLiveDevicesList

Dimi1010 · 2024-05-27T19:39:51Z

Pcap++/header/PcapLiveDeviceList.h



 /// @file

+#define PCPP_DEPRECATED_RAW_PTR_API PCPP_DEPRECATED("This method is deprecated in favor of the SmartPtrAPI overload.")


Doxygen generation really dislikes this macro for some reason, when used on a member function. No idea why... tried several things to fix them including mirroring the way the define is written in TcpLayer.h, but none of them worked. Would like a second opinion on why the generation fails on the macro.

Doxygen test fails with the following:

Preprocessing /tmp/cirrus-ci-build/Pcap++/head/tmp/cirrus-ci-build/Pcap++/header/PcapLiveDeviceList.h:62: error: Found ';' while parsing initializer list! (doxygen could be confused by a macro call without semicolon) (warning treated as error, aborting now) er/MBufRawPacket.h...

…DeviceListSearch, TestPcapLiveDevice, TestPcapLiveDeviceClone and TestRemoteCapture.

…smart-pointers # Conflicts: # Pcap++/header/PcapLiveDevice.h # Pcap++/src/PcapLiveDevice.cpp # Tests/Pcap++Test/Tests/LiveDeviceTests.cpp

…eviceList.

…smart-pointers

tigercosmos · 2024-06-03T05:07:26Z

Pcap++/header/DeviceUtils.h

@@ -0,0 +1,34 @@
+#pragma once


suggest to put all util functions in one file, as there is not much code.

Which one to merge into the other tho? I initially separated them as they have different use cases.

MemoryUtils is for general utilities w.r.t. smart pointer utilities and deleters.
DeviceUtils is for utility functions for Pcap devices, like fetching devices, etc.

MemoryUtils will be used a lot more as any time a smart pointer to a Pcap handle is used, it would need to be included for the deleter.

I see. originally I thought we could merge MemoryUtils and DeviceUtils, but it did make sense to be separated.

tigercosmos · 2024-06-03T05:10:09Z

Pcap++/header/PcapLiveDevice.h

+	 */
+	struct SmartPtrApiTag{};
+	/**
+	 * Helper tag constant for disambuguating smart pointer API.


typo: disambiguating, btw I rarely see this word, I wonder if it is precise here.

Fixed a95c946. I think it should be better now?

tigercosmos · 2024-06-03T05:11:39Z

Pcap++/header/PcapLiveDevice.h

@@ -130,6 +137,8 @@ namespace pcpp
 		static void onPacketArrivesNoCallback(uint8_t* user, const struct pcap_pkthdr* pkthdr, const uint8_t* packet);
 		static void onPacketArrivesBlockingMode(uint8_t* user, const struct pcap_pkthdr* pkthdr, const uint8_t* packet);
 	public:
+		PcapLiveDevice(const PcapLiveDevice& other) = delete;


please also implement moving construction/assignment.

btw, if the underlying are shared pointers, why do we disallow coping?

btw, if the underlying are shared pointers, why do we disallow coping?

IDK. I think the reason a LiveDevice itself is not copiable is something with the underlying Pcap descriptors. Although there is still the clone method so not really sure. The current change is mostly a stylistic from the pre-Cpp11 style of private ctors to post-Cpp11 deleted ctors.

we can keep the old clone, but I think we can now embrace the copy constructor/assignment?

Ah, I figured out at least partly why its private / deleted. LiveDevice is a virtual class and usually handled by pointers to base class (i.e. WinPcapLiveDevice device is used through PcapLiveDevice pointer). A copy ctor used from a base pointer will only use the base copy ctor, not the actual copy ctor of the class the base ptr points to.

please also implement moving construction/assignment.

Added 4bcb50e

Pcap++/header/PcapLiveDeviceList.h

Pcap++/src/PcapFilter.cpp

Pcap++/src/PcapLiveDevice.cpp

Pcap++/src/PcapLiveDeviceList.cpp

…smart-pointers

Dimi1010 · 2024-06-05T08:26:32Z

This PR will be split into multiple smaller PRs as part of #1431.

Dimi1010 added 5 commits May 6, 2024 15:11

Replaced the container of owning raw pointers of PcapDevices in PcapD…

5d10a89

…eviceList with smart pointers. Added a secondary vector of non-owning pointers to the devices to keep the signature of some getter methods as they return references.

Replaced raw pcap_if_t pointers with smart pointers in LiveDeviceList…

9146330

… initialization.

Simplified device insertion call.

bbbcd71

Rafactored while-loop into a for-loop.

19c93f3

Refactored to use a bytes view of IPv6 address when comparing the add…

9359c2c

…ress, instead of copying the address to a new bytes array.

Dimi1010 added 24 commits May 7, 2024 20:37

Merge branch 'dev' into feature/device-list-smart-pointers

226b313

Changed remote authentication structure in PcapRemoteDeviceList and P…

0ca74d9

…capRemoteDevice to be a shared_ptr.

Moved variable setup to initialization list.

276fdfb

C-cast -> Cpp cast.

2c1bfef

Replaced NULL with nullptr.

074de0a

Changed shared_ptr params to const ref

61fcfa8

Added a smart pointer version of getRemoteDeviceList.

0027113

Changed while-loop to for-loop.

bd1677f

Fixed possible copy from nullptr.

ca0602b

Added smart pointer factory overload for non-auth verison.

9551dbe

Changed setRemoteAuth to receive smart ptr by value as that enables m…

6cedf1b

…ore freedom on the caller, to skip a reference counter increase/decrease by moving.

Changed remote device to receive shared ptr by value, to enable move …

0a437be

…optimizations if the caller does not require his own ptr anymore

Added overload from authentication struct reference.

5f683c4

Cenrtalized deleters that do not need to be declared in the public he…

0a16197

…aders in MemoryUtils header.

Simplified conversion to string.

14c951f

Removed duplicated error buffer.

e8b0086

Changed interfaceList to be held by smart pointer.

b5cd2a2

Liter fixes.

412d5df

Refactored RemoteDeviceList to use RAII constructor initialization.

be3a169

Lint

820d4aa

Removed duplicate auth structure.

e0cdeda

Changed PcapRemoteDevice raw pointers to smart pointers for memory ma…

74d8be5

…nagement. Added a second helper 'view' vector to keep raw pointers of the devices for backward compat. Replaced iterator types with auto keyword. Removed destructor as the smart pointers handle cleanup.

Added API for getting PcapRemoteDevice as a shared pointer instead of…

e755209

… a non-owning raw pointer to allow the device to outlive the list if nessesary.

Lint

a83d9b1

Dimi1010 force-pushed the feature/device-list-smart-pointers branch from bb2cc09 to 541b198 Compare May 27, 2024 19:26

Dimi1010 commented May 27, 2024

View reviewed changes

Dimi1010 added 9 commits May 28, 2024 22:24

Added smart pointer api tests to TestPcapLiveDeviceList, TestPcapLive…

e9c7a9b

…DeviceListSearch, TestPcapLiveDevice, TestPcapLiveDeviceClone and TestRemoteCapture.

Interlaced tests in device list search.

9a4003e

Cleanup

65bc6e0

Merge remote-tracking branch 'upstream/dev' into feature/device-list-…

979273f

…smart-pointers # Conflicts: # Pcap++/header/PcapLiveDevice.h # Pcap++/src/PcapLiveDevice.cpp # Tests/Pcap++Test/Tests/LiveDeviceTests.cpp

Marked the device list view as mutable for consistency with PcapLiveD…

f6ba4a1

…eviceList.

Merge remote-tracking branch 'upstream/dev' into feature/device-list-…

8736f42

…smart-pointers

Changed clone internal to return smart pointer.

6e7f8cb

Fixed method not being const.

c9e37ee

Fixed duplicate clone.

2efeaec

Dimi1010 marked this pull request as ready for review June 2, 2024 18:49

Dimi1010 requested a review from seladb as a code owner June 2, 2024 18:49

Dimi1010 requested review from egecetin, clementperon and tigercosmos June 2, 2024 18:49

Dimi1010 added the refactoring label Jun 2, 2024

tigercosmos reviewed Jun 3, 2024

View reviewed changes

Dimi1010 added 8 commits June 3, 2024 21:33

Merge remote-tracking branch 'upstream/dev' into feature/device-list-…

39cee9b

…smart-pointers

Lint

0528e92

Improved documentation.

a95c946

Changed strcmp to std::strcmp

4ae2edb

Shortened double declaration to auto.

5d9dcfd

Documentation fixes.

5e25d9f

Marked move ctors/assignment as deleted.

4bcb50e

Merge remote-tracking branch 'upstream/dev' into feature/device-list-…

905f871

…smart-pointers

Dimi1010 marked this pull request as draft June 5, 2024 07:22

Dimi1010 closed this Jun 5, 2024

Dimi1010 mentioned this pull request Jun 6, 2024

Centralize code for fetching pcap devices. #1434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pcap device list refactoring to use smart pointers. #1386

Pcap device list refactoring to use smart pointers. #1386

Dimi1010 commented May 6, 2024 •

edited

Loading

Dimi1010 commented May 6, 2024 •

edited

Loading

Dimi1010 left a comment •

edited

Loading

Dimi1010 May 27, 2024 •

edited

Loading

tigercosmos Jun 3, 2024

Dimi1010 Jun 3, 2024 •

edited

Loading

tigercosmos Jun 3, 2024

Dimi1010 May 27, 2024

Dimi1010 May 27, 2024

Dimi1010 May 27, 2024

tigercosmos Jun 3, 2024

Dimi1010 Jun 3, 2024 •

edited

Loading

tigercosmos Jun 3, 2024

tigercosmos Jun 3, 2024

Dimi1010 Jun 3, 2024

tigercosmos Jun 3, 2024

tigercosmos Jun 3, 2024

Dimi1010 Jun 3, 2024 •

edited

Loading

tigercosmos Jun 3, 2024

Dimi1010 Jun 3, 2024 •

edited

Loading

Dimi1010 Jun 4, 2024

Dimi1010 commented Jun 5, 2024



		/// @file

		#define PCPP_DEPRECATED_RAW_PTR_API PCPP_DEPRECATED("This method is deprecated in favor of the SmartPtrAPI overload.")

Pcap device list refactoring to use smart pointers. #1386

Pcap device list refactoring to use smart pointers. #1386

Conversation

Dimi1010 commented May 6, 2024 • edited Loading

Dimi1010 commented May 6, 2024 • edited Loading

Dimi1010 left a comment • edited Loading

Choose a reason for hiding this comment

Dimi1010 May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimi1010 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimi1010 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimi1010 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimi1010 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimi1010 commented Jun 5, 2024

Dimi1010 commented May 6, 2024 •

edited

Loading

Dimi1010 commented May 6, 2024 •

edited

Loading

Dimi1010 left a comment •

edited

Loading

Dimi1010 May 27, 2024 •

edited

Loading

Dimi1010 Jun 3, 2024 •

edited

Loading

Dimi1010 Jun 3, 2024 •

edited

Loading

Dimi1010 Jun 3, 2024 •

edited

Loading

Dimi1010 Jun 3, 2024 •

edited

Loading