Use smart pointers for PcapLiveDeviceList's internal memory management. #1440
Conversation
Pcap++/header/PcapLiveDeviceList.h
Outdated
| @@ -23,20 +24,26 @@ namespace pcpp | |||
| class PcapLiveDeviceList | |||
| { | |||
| private: | |||
| std::vector<PcapLiveDevice*> m_LiveDeviceList; | |||
| std::vector<std::shared_ptr<PcapLiveDevice>> m_LiveDeviceList; | |||
There was a problem hiding this comment.
Why is this a vector of shared_ptr and not unique_ptr? 🤔
There was a problem hiding this comment.
This implementation uses shared pointers to store the live devices in preparation for point 4 of #1431.
From the PR description. I plan on having the API return shared pointers.
There was a problem hiding this comment.
I'm not sure I fully understand point 4, but I'm not sure there's a good reason to unify the APIs of PcapLiveDeviceList and PcapRemoteDeviceList, from multiple reasons:
PcapLiveDeviceListis a singleton because it finds all the interfaces on the machine. HoweverPcapRemoteDeviceListis not a singleton because it encapsulates remote devices on other machines so a user can have multiple instances of it. The API is quite different so not sure it's a good idea to merge it- Remote devices are a niche that isn't used a lot, whereas pcap devices is probably the most popular feature of PcapPlusPlus. I wouldn't mix between them
There was a problem hiding this comment.
Well, one of the reasons I started thinking about it is that both PcapLiveDeviceList and PcapRemoteDeviceList share a couple similarities and have some code duplication.
- Both store their devices in
vector<DevicePtr> - The methods
getPcapLiveDeviceByIpandgetPcapRemoteDeviceByIPhave practically duplicate implementation.
One of the reasons for unifying the API is having an internal base class template<DeviceType> PcapLiveDeviceListBase (name subject to change) that implements the getPcapDeviceByIP methods that currently have duplicate implementation.
The current methods could still be kept and just call the new functions too.
There was a problem hiding this comment.
Also, even if the API is not unified, I still have the proposal to get the API to return shared_ptr instead of raw pointers so you don't suddenly get invalid pointers. (refresh, or in case of RemoteDeviceList just the list going out of scope).
There was a problem hiding this comment.
to be honest, I wouldn't spend time on it because although there is some code duplication, it's not a lot and we can keep the code simple. If you really insist we can discuss it more...
There was a problem hiding this comment.
Hmm, ok then. Gonna shelve point 4 for now.
There was a problem hiding this comment.
PcapLiveDeviceList is a singleton
@seladb Btw, if the class is a singleton, why does it have a clone method?
There was a problem hiding this comment.
Changed to unique_ptr. ad77379
There was a problem hiding this comment.
PcapLiveDeviceList is a singleton
@seladb Btw, if the class is a singleton, why does it have a
clonemethod?
I agree this API is a bit weird 🙈
The reason is that users wanted to capture traffic from the same interface more than once, here is the GitHub issue: #766
The idea was to allow cloning a live device, or to allow cloning all devices...
|
@Dimi1010 please resolve the conflict. |
…v-list-internal-smart-pointers
Part of #1431. Partially implements point 1.
This PR implements usage of smart pointers for managing memory of member variables.
Dynamic memory management of translation unit local variables will be handled in a different PR.
This implementation uses shared pointers to store the live devices in preparation for point 4 of #1431.