PcapPlusPlus icon indicating copy to clipboard operation
PcapPlusPlus copied to clipboard

Pcap device list refactoring to use smart pointers.

Open Dimi1010 opened this issue 9 months ago • 1 comments

Part of #977.

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

Overview of changes:

  • [x] Refactor PcapLiveDeviceList to use smart pointers internally for memory management.
  • [x] Refactor PcapRemoteDeviceList to use smart pointers internally for memory management.
  • [x] Refactor PcapRemoteDevice to keep ~~its own RemoteAuthentication copy or~~ a shared reference with PcapRemoteDeviceList via std::shared_ptr as it currently uses a raw pointer to share authentication with the device list.
  • [x] Add shared_ptr overloads to PcapLiveDeviceList methods. Deprecate raw pointer methods.
  • [x] Add shared_ptr overloads to PcapRemoteDeviceList methods. Deprecate raw pointer methods.
  • [ ] Add unit tests for smart pointer API methods for PcapLiveDeviceList.
  • [ ] Add unit tests for smart pointer API methods for PcapRemoteDeviceList.
  • [ ] Find a better way to keep the old iterators than keeping a duplicate list?

Additional minor changes:

  • Optimized matching of IPv6 addresses to no longer copy the IPv6 address bytes.
  • Marked PcapLiveDeviceList and PCapRemoteDeviceList's copy ctors as explicitly deleted instead of keeping them private.
  • Moved internal helper deleters to MemoryUtils.h.

Dimi1010 avatar May 06 '24 12:05 Dimi1010

@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.

Dimi1010 avatar May 06 '24 13:05 Dimi1010

RemoteAuthentication

IMO, the unique copy makes sense, since once the authentication is set, it should never be possible to change by others.

tigercosmos avatar May 24 '24 00:05 tigercosmos

RemoteAuthentication

IMO, the unique copy makes sense, since once the authentication is set, it should never be possible to change by others.

Atm, made it so the user provides a unique_ptr to the factory method that is then internally changed to shared_ptr by the device list and shared with the list's devices. So all devices use the same auth object but it should not be exposed externally, outside the device list <-> device relationship.

Dimi1010 avatar May 24 '24 05:05 Dimi1010

RemoteAuthentication

IMO, the unique copy makes sense, since once the authentication is set, it should never be possible to change by others.

Atm, made it so the user provides a unique_ptr to the factory method that is then internally changed to shared_ptr by the device list and shared with the list's devices. So all devices use the same auth object but it should not be exposed externally, outside the device list <-> device relationship.

True, as PcapRemoteDeviceList doesn't expose PcapRemoteAuthentication, PcapRemoteAuthentication can directly be a shared object among the list and device.

tigercosmos avatar May 24 '24 06:05 tigercosmos

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

Dimi1010 avatar Jun 05 '24 08:06 Dimi1010