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

PcapRemoteDevice's clone method does not work. #1439

Closed
Dimi1010 opened this issue Jun 6, 2024 · 5 comments · Fixed by #1497
Closed

PcapRemoteDevice's clone method does not work. #1439

Dimi1010 opened this issue Jun 6, 2024 · 5 comments · Fixed by #1497
Assignees
Labels

Comments

@Dimi1010
Copy link
Collaborator

Dimi1010 commented Jun 6, 2024

PcapRemoteDevice's clone method directly inherits from PcapLiveDevice's clone method.
The method incorrectly attempts to fetch the device from local devices, failing and returning nullptr.

Solution would be to add an override to the clone method, correctly dispatching to fetch the remote devices, or refactoring the clone call to skip the fetching if possible.

@seladb
Copy link
Owner

seladb commented Jun 24, 2024

@Dimi1010 I can't remember if this issue was fixed or not 🤦‍♂️

@Dimi1010
Copy link
Collaborator Author

Dimi1010 commented Jun 24, 2024

Still present, as the remote device class does not have its own clone override. I was mostly waiting for the clang format PRs to be done with before I start working on it.

@seladb
Copy link
Owner

seladb commented Jun 24, 2024

Got it, thanks. The clang-formate might take time because we're moving in small steps. We'll probably save Pcap++ to last because it's the most complex as not everything has tests...
Feel free to start working on it if you have time

@Dimi1010
Copy link
Collaborator Author

Dimi1010 commented Jun 24, 2024

Sure.

Btw @seladb , is there a specific reason we are cloning the devices via re-fetching the pcap_if_t struct? As far as I can see, the struct is only read for the data it has in the constructor, so a simpler private copy constructor could probably be implemented to be used in clone instead of re-fetching all devices.

@Dimi1010
Copy link
Collaborator Author

Fixed by #1497

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

Successfully merging a pull request may close this issue.

2 participants