Skip to content

Refactored PcapHandle to utilize std::unique_ptr internally. #1753

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.

Sign up for GitHub

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

Closed
Dimi1010 wants to merge 2 commits into from
Closed

Refactored PcapHandle to utilize std::unique_ptr internally. #1753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40b5172
Refactored PcapHandle to utilize std::unique_ptr + PcapCloseDeleter i…
Dimi1010 Mar 28, 2025
abced57
Fixed PcapUtils.h needing to be public header.
Dimi1010 Mar 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Pcap++/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ set(
header/PcapFilter.h
header/PcapLiveDevice.h
header/PcapLiveDeviceList.h
header/PcapUtils.h
Copy link
Owner

@seladb seladb Mar 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid exposing internal header files like PcapUtils.h.

The reason it is needed is because we use internal::PcapCloseDeleter inside a "public" header file, and we should find a way around it.

I tried using forward declaration of internal::PcapCloseDeleter but couldn't make it work so far. @Dimi1010 maybe you can make it work?

Other options are:

  • Move internal::PcapCloseDeleter to PcapDevice.h - not ideal, but I think it'll work
  • Define m_PcapDescriptor as "no type" like void* and assign the type in the .cpp file. However it's a pretty ugly workaround

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Mar 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this as a potential solution. I think this is the cleanest, tbh.
#1753 (comment)

internal::PcapCloseDeleter can't work as a forward declaration here as it is default constructed in the std::unique_ptr so it needs a concrete type.

Copy link
Owner

@seladb seladb Mar 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think exposing an internal folder of headers is not a good solution either because exposing header files just because we have some technical limitations is not something we want. I think we should either find a workaround (even if it's not very nice), or just leave the code as it is

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen that pattern be used to separate the stable public headers from the unstable ones, that can change at any moment and users shouldn't rely upon. Most notably I have seen it in many boost libraries ("details" folders).

Moving internal::PcapCloseDeleter to PcapDevice.h creates a dependency between PcapFilter.cpp and PcapDevice.h, which I would prefer to not have.

I guess we can leave this as is for now tho.

Copy link
Owner

@seladb seladb Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it was "stable" vs "unstable" public API I'd agree that separating them into different folders could work. But exposing a "private API" (internal code) is probably not the best way to go.

I guess leaving it as is is fine if we can find a better solution...

header/RawSocketDevice.h
)

Expand Down
11 changes: 7 additions & 4 deletions Pcap++/header/PcapDevice.h
View file
Open in desktop
Copy link
Collaborator Author

@Dimi1010 Dimi1010 Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have an /internal/ folder for headers that need to be public for compilation but shouldn't be included by users directly?

Copy link
Collaborator

@tigercosmos tigercosmos Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with the current way, but it's also fine to move the files.

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels cleaner to have the "internal" public headers separated. That way, we don't have to worry about breaking changes if "internal" files are renamed or moved around due to people directly including them. And if they still do, that is on them.

Copy link
Collaborator

@tigercosmos tigercosmos Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I agree with you. Just think we can put effort on more important parts (if there are such things)

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, it's possibly not the most important. But since this PR exposes a new public header (PcapUtils.h), that only defines internal symbols, I would prefer to have that decided now instead of maybe having to do a transition later, when it has already been placed next to the user API headers.

Copy link
Collaborator

@tigercosmos tigercosmos Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I don't have strong opion on this. Let see how @seladb says.

Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
#pragma once

#include <memory>

#include "Device.h"
#include "PcapUtils.h"

// forward declaration for the pcap descriptor defined in pcap.h
struct pcap;
Expand Down Expand Up @@ -31,10 +34,10 @@ namespace pcpp
explicit PcapHandle(pcap_t* pcapDescriptor) noexcept;

PcapHandle(const PcapHandle&) = delete;
PcapHandle(PcapHandle&& other) noexcept;
PcapHandle(PcapHandle&& other) noexcept = default;

PcapHandle& operator=(const PcapHandle&) = delete;
PcapHandle& operator=(PcapHandle&& other) noexcept;
PcapHandle& operator=(PcapHandle&& other) noexcept = default;
PcapHandle& operator=(std::nullptr_t) noexcept;

~PcapHandle();
Expand All @@ -48,7 +51,7 @@ namespace pcpp
/// @return The underlying pcap descriptor.
pcap_t* get() const noexcept
{
return m_PcapDescriptor;
return m_PcapDescriptor.get();
}

/// @brief Releases ownership of the handle and returns the pcap descriptor.
Expand Down Expand Up @@ -81,7 +84,7 @@ namespace pcpp
}

private:
pcap_t* m_PcapDescriptor = nullptr;
std::unique_ptr<pcap_t, internal::PcapCloseDeleter> m_PcapDescriptor;
};
} // namespace internal

Expand Down
28 changes: 3 additions & 25 deletions Pcap++/src/PcapDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,6 @@ namespace pcpp
PcapHandle::PcapHandle(pcap_t* pcapDescriptor) noexcept : m_PcapDescriptor(pcapDescriptor)
{}

PcapHandle::PcapHandle(PcapHandle&& other) noexcept : m_PcapDescriptor(other.m_PcapDescriptor)
{
other.m_PcapDescriptor = nullptr;
}

PcapHandle& PcapHandle::operator=(PcapHandle&& other) noexcept
{
if (this != &other)
{
reset(other.m_PcapDescriptor);
other.m_PcapDescriptor = nullptr;
}
return *this;
}

PcapHandle& PcapHandle::operator=(std::nullptr_t) noexcept
{
reset();
Expand All @@ -39,19 +24,12 @@ namespace pcpp

pcap_t* PcapHandle::release() noexcept
{
auto result = m_PcapDescriptor;
m_PcapDescriptor = nullptr;
return result;
return m_PcapDescriptor.release();
}

void PcapHandle::reset(pcap_t* pcapDescriptor) noexcept
{
pcap_t* oldDescriptor = m_PcapDescriptor;
m_PcapDescriptor = pcapDescriptor;
if (oldDescriptor != nullptr)
{
pcap_close(oldDescriptor);
}
m_PcapDescriptor.reset(pcapDescriptor);
}

char const* PcapHandle::getLastError() const noexcept
Expand All @@ -62,7 +40,7 @@ namespace pcpp
return noHandleError;
}

return pcap_geterr(m_PcapDescriptor);
return pcap_geterr(m_PcapDescriptor.get());
}
} // namespace internal

Expand Down
Loading