Added sized overloads for buffer construction of IPv6Address and MacAddress.
#1847
Conversation
…Address. Deprecated allocating copyTo method.
Dimi1010
added
refactoring
API deprecation
Common++/header/IpAddress.h
Outdated
| /// @brief Allocates a byte array and copies address value into it. | ||
| /// @param[out] size Returns the size in bytes of the allocated array. Usually 16. | ||
| /// @return A unique pointer to the allocated byte array containing the address value | ||
| std::unique_ptr<uint8_t[]> copyToNewBuffer(size_t& size) const; |
There was a problem hiding this comment.
Debating if its better to return unique_ptr with size as an out param, or return pair<unique_ptr<...>, size_t>
PS: Kinda starting to lean to pair as it will keep the buffer and size logically grouped? 🤔
There was a problem hiding this comment.
We can consider to use std::array here as well?
There was a problem hiding this comment.
There is toByteArray that returns an std::array type.
This one mostly covers existing functionality that was in copyTo that allocates heap buffer. Which I suppose can be cheaper if it will be changing ownership around a lot?
There was a problem hiding this comment.
I am okay with having std::unique_ptr API, but I wonder if people need it. IMO, most of the time, people may only need the reference to the bytes directly from the IpAddress object. In rare cases, they can just copy the bytes. Then, we can keep the interface as simple as possible.
There was a problem hiding this comment.
That is true. I can't find a lot of usages where a new buffer is required. Most of the time it is copied to an already existing buffer, if the copyTo api is used.
There was a problem hiding this comment.
Ended up deprecating the allocating copyTo API altogether. It is not commonly used and it is clunky IMO.
The use case can be handled by the new size_t copyTo(uint8_t* buffer, size_t size) function that returns the written bytes or the required size. It also supports querying the function.
Example usage:
MacAddress mac;
size_t addrLen = mac.copyTo(nullptr, 0); // Query the function for a required buffer.
uint8_t* addr = new uint8_t[addrLen];
if(mac.copyTo(addr, addrLen) != addrLen) { /* Error check */ }
// Use addr...
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1847 +/- ##
==========================================
+ Coverage 83.21% 83.24% +0.02%
==========================================
Files 285 285
Lines 49835 49946 +111
Branches 10556 10779 +223
==========================================
+ Hits 41472 41576 +104
- Misses 7552 7567 +15
+ Partials 811 803 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Updated non-allocating copyTo API to return written bytes.
Common++/header/IpAddress.h
Outdated
| /// @deprecated Allocating copyTo API is deprecated. | ||
| PCPP_DEPRECATED("Allocating copyTo API is deprecated.") |
There was a problem hiding this comment.
Why do we want to deprecate this API?
If someone wants the library to allocate the buffer for them, what's wrong with it? 🤔
There was a problem hiding this comment.
It is very similar to the new size_t copyTo(uint8_t*, size_t) function that performs a pure copy to the input buffer. The only difference is that one is a pointer and the other a pointer to pointer param.
I also don't think it should be the copyTo function's job to allocate a new buffer. IMO, it breaks the SRP. I understand that is was needed because of how the previous copyTo function worked, it did not provide a way to check the required size of a buffer without the user knowing its N bytes.
The new function allows reporting of the required buffer size (query mode), so it should be left to the user to handle the memory management of how he wants to provide the required buffer. It allows for simpler, more consistent API of the class and less potential for errors due to the class allocating a buffer the user did not expect.
There was a problem hiding this comment.
The only difference is that one is a pointer and the other a pointer to pointer param.
It also allocates a new buffer before copying the data to it, which is the main difference between the 2 overloads.
I also don't think it should be the
copyTofunction's job to allocate a new buffer. IMO, it breaks the SRP. I understand that is was needed because of how the previouscopyTofunction worked, it did not provide a way to check the required size of a buffer without the user knowing its N bytes.
The user still needs to know the required size to allocate enough bytes before calling this method. Maybe in some cases, users want the library to do that for them.
There was a problem hiding this comment.
It also allocates a new buffer before copying the data to it, which is the main difference between the 2 overloads.
This was meant as the only difference between the two signatures is a degree of indirection, so it will be easy to mistake one for the other.
I also don't think it should be the
copyTofunction's job to allocate a new buffer. IMO, it breaks the SRP. I understand that was needed because of how the previouscopyTofunction worked, it did not provide a way to check the required size of a buffer without the user knowing its N bytes.The user still needs to know the required size to allocate enough bytes before calling this method. Maybe in some cases, users want the library to do that for them.
Well, an earlier commit had the functionality remade to copyToNewBuffer that returned an unique ptr<uint8_t[]>, but the API for it ended up kinda clunky. I could re-add that, if needed.
Yes, they still need to know it, but they can control the method of allocation. It follows the somewhat standard pattern of query -> allocate -> populate. I personally dislike black box allocations that then rely on the user knowing the precice method the library allocated the memory inside the function.
Usually, a black box allocation function is accompanied by a black box deallocation function. In this case, there is only an allocation function.
There was a problem hiding this comment.
I'm pretty sure I saw this pattern somewhere where uint8_t** means that the buffer is allocated by the library. However I'm not sure where I saw it (I wrote these methods quite a few year ago).
All I'm saying is that I don't think we should deprecate (and later remove) these methods. We can just keep them as they don't break anything and might be useful for some people.
There was a problem hiding this comment.
I see. I have seen that pattern, yes.
My main concern is that copyTo(uint8_t** ptr, size_t& size) and copyTo(uint8_t* ptr, size_t size) can be easily mistakenly called.
For example, a user wants to copy to a new buffer but forgets to call with &ptr, and instead calls with ptr. The call will be resolved to the non allocating overload that will either do a no-op on nullptr or explode if the ptr is uninitialized.
Perhaps we can keep the operation but rename the method to something like copyToNewBuffer?
There was a problem hiding this comment.
I agree that we should just keep one. IMO, **ptr looks more like C-style.
There was a problem hiding this comment.
Update 019b376: Re-added allocating copyTo with the current signature under the name copyToNewBuffer(uint8_t** buffer, size_t& size) and updated deprecations.
Common++/header/IpAddress.h
Outdated
| return requiredSize; | ||
| } | ||
|
|
||
| if (size < requiredSize) |
There was a problem hiding this comment.
do we want to support an array size of less than 16 bytes? I'm not sure if there a need to copy just a portion of an IPv6 address
There was a problem hiding this comment.
It doesn't support copying to an array of less than 16 bytes. If the buffer size is less, no copy is made, and the required buffer size is returned.
If the array is larger, the size of the array that has been written to is returned.
Essentially:
- If the return value is greater than the size input of the buffer, a buffer of at least the size of the return value is needed.
- If the return value is less or equal to the input size param, that is the number of bytes that are updated.
That patern allows runtime querying of the required buffer size without knowing and hardcoding it.
There was a problem hiding this comment.
I'm sorry, I misread the code. I find this API a bit confusing... as a user, I allocate a byte array of a certain size, then call this method. Now, in order to know if the copy succeeded or not, I need to check the return size and compare it to the size I provided to the function. I guess throwing an exception is better in this case...
There was a problem hiding this comment.
Well, it is a pattern that allows the user to query the precice required size and not have to guestimate / handcode the allocation size.
Common use case is:
- Call the method with nullptr + 0 size -> get required bytes
- Allocate buffer
- Call method with (buffer, bufSize)
- Check if the return value is less or equal to bufferSize.
It also allows the user to guestimate the buffer if there is a common case. But if for some reason, the initial buffer is insufficient, the pattern allows for hot retry with the precise amount without triggering the exception mechanism. Here, its value is kinda diminished because the buffer is always 16, but it shines when the required buffer is dynamic in range.
There was a problem hiding this comment.
I think IP addresses are well known concepts, and even more so for people who use these classes. They should be aware that IPv4 is 4 bytes and IPv6 is 16 bytes. IMHO throwing an exception here should be good enough
There was a problem hiding this comment.
In this case, why not check if the size is either 4 or 16?
There was a problem hiding this comment.
I agree that they are well known concepts. If the size is known, step 1 can be skipped and the buffer can be directly allocated / used. I think its a good idea to still have a way to query the size at runtime, even if it should be well known.
Additionally, I counted copyTo as a low-level operation, so I tried to minimize the exception throwing for the most common error case (insufficient buffer). Ideally, I made the API that way to keep the signature independent of the actual type that is being copied, so we can have a unified copyTo API signature for both dynamic and static length objects, thus allowing more flexibility down the line if needed. The initial signature comes from size_t writeToBuffer(uint8_t* buffer, size_t bufferLen) const; in #1820, where it was used to serialize helper structs.
In this case, why not check if the size is either 4 or 16?
The checks are not precise 4 or 16 so it can potentially write to overprovisioned buffers. That is one of primary reasons the function returns the num of written bytes, instead of just true/false on success.
Common++/header/MacAddress.h
Outdated
| /// @deprecated Allocating copyTo API is deprecated. | ||
| PCPP_DEPRECATED("Allocating copyTo API is deprecated.") |
Common++/header/MacAddress.h
Outdated
| return requiredSize; | ||
| } | ||
|
|
||
| if (size < m_Address.size()) |
There was a problem hiding this comment.
ditto: do we need to support copying to an array smaller than 6 bytes?
IPv6Address(const uint8_t* buf, size_t size)MacAddress(const uint8_t* buf, size_t size)copyTosize_t IPv6Address::copyTo(uint8_t* buf, size_t size)size_t MacAddress::copyTo(uint8_t* buf, size_t size)copyTo(uint8_t** buf)andcopyTo(uint8_t** buf, size_t& size)withcopyToNewBuffer.