spanification: automatically spanify remoting/protocol/pseudo_tcp.cc etc. [chromium/src : main]
Daniel Angulo (Gerrit)
Daniel Angulo voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 15 comments![]( "Open in Gerrit")
std::unique_ptr<char[]> buffer_ GUARDED_BY(lock_);While we are here switch this to a base::HeapArray? and remove `buffer_length_`? (still need data_length_ most likely).
auto bytes = base::U32ToBigEndian(val);Because this is network code might be best to do
`base::as_bytes(base::HostToNet32(val));`
Thoughts? Here and below?
I agree that new is BigEndian so that is fine, but they have a nice helper function and it probably makes more sense to keep it consistent?
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))We can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
const uint8_t* Data() const { return data_.subspan(pos_).data(); }`&data_[pos_]`
is better I think?
// SAFETY: `buffer` is a raw C-style array of size `len`, so we can safely
// reinterpret the pointer as a span of bytes.The safety concern isn't from the reinterpret it is from no knowing if the length is correct. Same as above we can't use UNSAFE_BUFFERS here.
std::unique_ptr<uint8_t[]> buffer(new uint8_t[MAX_PACKET]);Switch this to a HeapArray then you can get a span directly.
buffer_span.first<4u>().copy_from(base::U32ToBigEndian(m_conv));
buffer_span.subspan(4u).first<4u>().copy_from(base::U32ToBigEndian(seq));
buffer_span.subspan(8u).first<4u>().copy_from(
base::U32ToBigEndian(m_rcv_nxt));
buffer_span[12] = 0;
buffer_span[13] = flags;
buffer_span.subspan(14u).first<2u>().copy_from(
base::U16ToBigEndian(static_cast<uint16_t>(m_rcv_wnd >> m_rwnd_scale)));
// Timestamp computations
buffer_span.subspan(16u).first<4u>().copy_from(base::U32ToBigEndian(now));
buffer_span.subspan(20u).first<4u>().copy_from(
base::U32ToBigEndian(m_ts_recent));This looks like a SpanWriter?
seg.conv = base::U32FromBigEndian(buffer.first<4u>());
seg.seq = base::U32FromBigEndian(buffer.subspan(4u).first<4u>());
seg.ack = base::U32FromBigEndian(buffer.subspan(8u).first<4u>());
seg.flags = buffer[13];
seg.wnd = base::U16FromBigEndian(buffer.subspan(14u).first<2u>());
seg.tsval = base::U32FromBigEndian(buffer.subspan(16u).first<4u>());
seg.tsecr = base::U32FromBigEndian(buffer.subspan(20u).first<4u>());This looks like a spanReader?
} else if (seg.data.size() != 0) {nit: !seq.data.empty()
seg.data = seg.data.first(seg.data.size() - nAdjust);I think you are double moving the buffer here?
```
seg.data += nAdjust;
seg.len -= nAdjust;
```
The old code moved the buffer and then decreased the length by the same amount (to keep them in sync).
Your new code moves the buffer forward by nAdjust and then takes a further nAdjust off in addition (this is incorrect).
Delete the seq.data.first line?
if (!m_rbuf.WriteOffset(seg.data.data(), seg.data.size(), nOffset,Can we update WriteOffset?
bool PseudoTcp::LockedFifoBuffer::WriteOffset(const void* buffer,
size_t bytes,It looks like we call this function with a span could we update this one?
bool PseudoTcp::LockedFifoBuffer::Read(void* buffer,
size_t bytes,Can we check all of these?
dest(reinterpret_cast<char*>(buffer.data()), buffer.size()));base::as_writable_chars(buffer) should work no?
base::span<const char> UNSAFE_BUFFERS(
src(&buffer_[read_position], tail_copy));
dest.first(tail_copy).copy_from(src);
base::span<const char> UNSAFE_BUFFERS(src2(&buffer_[0], copy - tail_copy));base::as_chars?
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 15 comments![]( "Open in Gerrit")
While we are here switch this to a base::HeapArray? and remove `buffer_length_`? (still need data_length_ most likely).
Done
Because this is network code might be best to do
`base::as_bytes(base::HostToNet32(val));`
Thoughts? Here and below?
I agree that new is BigEndian so that is fine, but they have a nice helper function and it probably makes more sense to keep it consistent?
Done
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))We can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
I was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
const uint8_t* Data() const { return data_.subspan(pos_).data(); }`&data_[pos_]`
is better I think?
Done
// SAFETY: `buffer` is a raw C-style array of size `len`, so we can safely
// reinterpret the pointer as a span of bytes.The safety concern isn't from the reinterpret it is from no knowing if the length is correct. Same as above we can't use UNSAFE_BUFFERS here.
Done
std::unique_ptr<uint8_t[]> buffer(new uint8_t[MAX_PACKET]);Switch this to a HeapArray then you can get a span directly.
Done
buffer_span.first<4u>().copy_from(base::U32ToBigEndian(m_conv));
buffer_span.subspan(4u).first<4u>().copy_from(base::U32ToBigEndian(seq));
buffer_span.subspan(8u).first<4u>().copy_from(
base::U32ToBigEndian(m_rcv_nxt));
buffer_span[12] = 0;
buffer_span[13] = flags;
buffer_span.subspan(14u).first<2u>().copy_from(
base::U16ToBigEndian(static_cast<uint16_t>(m_rcv_wnd >> m_rwnd_scale)));
// Timestamp computations
buffer_span.subspan(16u).first<4u>().copy_from(base::U32ToBigEndian(now));
buffer_span.subspan(20u).first<4u>().copy_from(
base::U32ToBigEndian(m_ts_recent));This looks like a SpanWriter?
Done
seg.conv = base::U32FromBigEndian(buffer.first<4u>());
seg.seq = base::U32FromBigEndian(buffer.subspan(4u).first<4u>());
seg.ack = base::U32FromBigEndian(buffer.subspan(8u).first<4u>());
seg.flags = buffer[13];
seg.wnd = base::U16FromBigEndian(buffer.subspan(14u).first<2u>());
seg.tsval = base::U32FromBigEndian(buffer.subspan(16u).first<4u>());
seg.tsecr = base::U32FromBigEndian(buffer.subspan(20u).first<4u>());This looks like a spanReader?
Done
} else if (seg.data.size() != 0) {Daniel Angulonit: !seq.data.empty()
Done
I think you are double moving the buffer here?
```
seg.data += nAdjust;
seg.len -= nAdjust;
```The old code moved the buffer and then decreased the length by the same amount (to keep them in sync).
Your new code moves the buffer forward by nAdjust and then takes a further nAdjust off in addition (this is incorrect).
Delete the seq.data.first line?
Done
if (!m_rbuf.WriteOffset(seg.data.data(), seg.data.size(), nOffset,Daniel AnguloCan we update WriteOffset?
Done
bool PseudoTcp::LockedFifoBuffer::WriteOffset(const void* buffer,
size_t bytes,It looks like we call this function with a span could we update this one?
Done
bool PseudoTcp::LockedFifoBuffer::Read(void* buffer,
size_t bytes,Can we check all of these?
Done
dest(reinterpret_cast<char*>(buffer.data()), buffer.size()));base::as_writable_chars(buffer) should work no?
Done
base::span<const char> UNSAFE_BUFFERS(
src(&buffer_[read_position], tail_copy));
dest.first(tail_copy).copy_from(src);
base::span<const char> UNSAFE_BUFFERS(src2(&buffer_[0], copy - tail_copy));- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 7 comments![]( "Open in Gerrit")
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))Daniel AnguloWe can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
I was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
If this function isn't used we should just delete it I think?
I guess the API it is copied from has one, so we could keep it.
Also I think std::string_view is fine? we are writing a string so accepting a string and converting it to bytes seems fine?
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(buffer, len))),We can't use UNSAFE_BUFFERS here still, we have the same issue as the other location.
uint32_t PseudoTcp::queue(const char* data, uint32_t len, bool bCtrl) {Can we make this a span as well?
reader.Skip(1u);What is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
parseOptions(seg.data.subspan(1u));nit: template subspan<1u>
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
new_buffer.subspan(0, tail_copy)
.copy_from(buffer_.subspan(read_position_, tail_copy));
new_buffer.subspan(tail_copy, copy - tail_copy)
.copy_from(buffer_.subspan(0, copy - tail_copy));could this be written as a base::span::split? I think so?
base::span<char> dest = base::as_writable_chars(buffer);
base::span<const char> src =
base::as_chars(buffer_.subspan(read_position, tail_copy));
dest.first(tail_copy).copy_from(src);
base::span<const char> src2 =
base::as_chars(buffer_.subspan(0, copy - tail_copy));
dest.subspan(tail_copy, copy - tail_copy).copy_from(src2);I didn't fully math this out but also feels like a split of some sort?
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 7 comments![]( "Open in Gerrit")
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))Daniel AnguloWe can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
Stephen NuskoI was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
If this function isn't used we should just delete it I think?
I guess the API it is copied from has one, so we could keep it.
Also I think std::string_view is fine? we are writing a string so accepting a string and converting it to bytes seems fine?
isn't it writing data+pos into the same str that is the parameter?
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(buffer, len))),We can't use UNSAFE_BUFFERS here still, we have the same issue as the other location.
Done
uint32_t PseudoTcp::queue(const char* data, uint32_t len, bool bCtrl) {Can we make this a span as well?
Done
reader.Skip(1u);What is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
in the writter we have buff[12] = 0; (which is the position 13) then buff[13] = flags (which is position 14). If I disable the skip then a bunch of tests start to fail
parseOptions(seg.data.subspan(1u));Daniel Angulonit: template subspan<1u>
Done
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
new_buffer.subspan(0, tail_copy)
.copy_from(buffer_.subspan(read_position_, tail_copy));
new_buffer.subspan(tail_copy, copy - tail_copy)
.copy_from(buffer_.subspan(0, copy - tail_copy));could this be written as a base::span::split? I think so?
Done
base::span<char> dest = base::as_writable_chars(buffer);
base::span<const char> src =
base::as_chars(buffer_.subspan(read_position, tail_copy));
dest.first(tail_copy).copy_from(src);
base::span<const char> src2 =
base::as_chars(buffer_.subspan(0, copy - tail_copy));
dest.subspan(tail_copy, copy - tail_copy).copy_from(src2);I didn't fully math this out but also feels like a split of some sort?
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 11 comments![]( "Open in Gerrit")
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))Daniel AnguloWe can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
Stephen NuskoI was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
Daniel AnguloIf this function isn't used we should just delete it I think?
I guess the API it is copied from has one, so we could keep it.
Also I think std::string_view is fine? we are writing a string so accepting a string and converting it to bytes seems fine?
isn't it writing data+pos into the same str that is the parameter?
Obviously a char span works, but an std::string_view can be converted to a span cheaply so I'm not sure I follow the concern?
Since you already have to do base::as_writable_byte_span there is no difference but API clarity that you have a string? So better to use an std::string_view no?
// SAFETY: `str` is a raw char*, but we immediately convert it to a span of
// bytes, which makes our intentions clear and prevents buffer overflows.No need for a safety comment.
base::span<uint8_t> buffer(buffer_storage);You don't need this variable, just pass `buffer_storage` directly into the SpanWriter.
And use the `subspan` method on `buffer_storage` directly for ReadOffset call.
reader.Skip(1u);Daniel AnguloWhat is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
in the writter we have buff[12] = 0; (which is the position 13) then buff[13] = flags (which is position 14). If I disable the skip then a bunch of tests start to fail
Perfect can you add a comment mentioning this? Makes sense.
// SAFETY: buf.Data() is a pointer to uint8_t of size buf.Length(), and we
// cast it to a pointer to char of that size.
queue(UNSAFE_BUFFERS(base::span(reinterpret_cast<const char*>(buf.Data()),
static_cast<uint32_t>(buf.Length()))),We own the ByteBufferWriter type here in our code, and are already modifying it. Can we just add a data() and size() method to it so it can directly construct a span and then we can use `as_writable_bytes`?
Then remove the SAFETY comment.
new_buffer_first.copy_from(buffer_.subspan(read_position_, tail_copy));copy_from_nonoverlapping (these are separate heap arrays they can't be the same memory).
const size_t tail_copy = std::min(copy, buffer_.size() - write_position);
buffer_.subspan(write_position, tail_copy)
.copy_from(base::as_chars(buffer.first(tail_copy)));
buffer_.first(copy - tail_copy)
.copy_from(base::as_chars(buffer.subspan(tail_copy, copy - tail_copy)));Switch this into a split as well?
UNSAFE_BUFFERS(base::span(packet.c_str(), packet.size())));We don't need this UNSAFE_BUFFERS
Just make a string_view or use `base::as_writable_chars`
received = remote_.Recv(base::span(block).first(block.size()));first(block.size) of a span of block doesn't make any difference. Remove .first()
received = receiver_->Recv(base::span(block).first(block.size()));Also review this and other calls for redundant first calls.
base::span(buffer->data(), static_cast<size_t>(buffer_size))));Can't you juse use `buffer.span()`? here and below?
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Thanks!
(I am waiting for the current suggestion to be addressed + the build to pass)
Feel free to close this comment when this patch is ready to be reviewed again.
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 13 comments![]( "Open in Gerrit")
Thanks!
(I am waiting for the current suggestion to be addressed + the build to pass)
Feel free to close this comment when this patch is ready to be reviewed again.
Done
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))Daniel AnguloWe can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
Stephen NuskoI was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
Daniel AnguloIf this function isn't used we should just delete it I think?
I guess the API it is copied from has one, so we could keep it.
Also I think std::string_view is fine? we are writing a string so accepting a string and converting it to bytes seems fine?
Stephen Nuskoisn't it writing data+pos into the same str that is the parameter?
Obviously a char span works, but an std::string_view can be converted to a span cheaply so I'm not sure I follow the concern?
Since you already have to do base::as_writable_byte_span there is no difference but API clarity that you have a string? So better to use an std::string_view no?
base::as_writable_byte_span cannot be used with a std::string_view. Here's why
std::string_view is Read-Only: A std::string_view (and absl::string_view) provides a non-owning, read-only view into an existing character sequence. It doesn't own the data and cannot be used to modify it.
// SAFETY: `str` is a raw char*, but we immediately convert it to a span of
// bytes, which makes our intentions clear and prevents buffer overflows.No need for a safety comment.
Done
You don't need this variable, just pass `buffer_storage` directly into the SpanWriter.
And use the `subspan` method on `buffer_storage` directly for ReadOffset call.
no viable constructor or deduction guide for deduction of template arguments of 'base::SpanWriter'
589 | base::SpanWriter writer(buffer_storage);
reader.Skip(1u);Daniel AnguloWhat is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
Stephen Nuskoin the writter we have buff[12] = 0; (which is the position 13) then buff[13] = flags (which is position 14). If I disable the skip then a bunch of tests start to fail
Perfect can you add a comment mentioning this? Makes sense.
Done
// SAFETY: buf.Data() is a pointer to uint8_t of size buf.Length(), and we
// cast it to a pointer to char of that size.
queue(UNSAFE_BUFFERS(base::span(reinterpret_cast<const char*>(buf.Data()),
static_cast<uint32_t>(buf.Length()))),We own the ByteBufferWriter type here in our code, and are already modifying it. Can we just add a data() and size() method to it so it can directly construct a span and then we can use `as_writable_bytes`?
Then remove the SAFETY comment.
Done
new_buffer_first.copy_from(buffer_.subspan(read_position_, tail_copy));copy_from_nonoverlapping (these are separate heap arrays they can't be the same memory).
Done
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
new_buffer.subspan(0, tail_copy)
.copy_from(buffer_.subspan(read_position_, tail_copy));
new_buffer.subspan(tail_copy, copy - tail_copy)
.copy_from(buffer_.subspan(0, copy - tail_copy));Daniel Angulocould this be written as a base::span::split? I think so?
Done
how? I see that there could be a split at tail_copy, but buffer_.subspan(0, copy - tail_copy) starts at offset 0 not at tail_copy, maybe I'm just a little bit confused but I don't see a clean split
const size_t tail_copy = std::min(copy, buffer_.size() - write_position);
buffer_.subspan(write_position, tail_copy)
.copy_from(base::as_chars(buffer.first(tail_copy)));
buffer_.first(copy - tail_copy)
.copy_from(base::as_chars(buffer.subspan(tail_copy, copy - tail_copy)));Switch this into a split as well?
Done
UNSAFE_BUFFERS(base::span(packet.c_str(), packet.size())));We don't need this UNSAFE_BUFFERS
Just make a string_view or use `base::as_writable_chars`
Done
received = remote_.Recv(base::span(block).first(block.size()));first(block.size) of a span of block doesn't make any difference. Remove .first()
Done
received = receiver_->Recv(base::span(block).first(block.size()));Also review this and other calls for redundant first calls.
Done
base::span(buffer->data(), static_cast<size_t>(buffer_size))));Can't you juse use `buffer.span()`? here and below?
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 6 comments![]( "Open in Gerrit")
int PseudoTcp::Recv(base::span<char> buffer) {Could this be uint8_t?
if (!m_rbuf.Read(base::as_writable_bytes(buffer), &read)) {This can be removed after applying my previous comment.
int buffer_size,Can we `CHECK_EQ(buffer->span().size(), buffer_size)`.
Eventually, the `buffer_size` argument would be removed, but this is going to take additional refactoring.
// SAFETY: buffer_size is the size of the buffer, and the buffer is a pointer
// to char of that size.We don't need a "safety" comment here. There are no UNSAFE_BUFERS
int result = pseudo_tcp_.Recv(base::as_writable_chars(buffer->span()));This can be removed if you apply one of my other comments about passing `base::span<uint8_t>`
Related details
- Daniel Angulo
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 5 comments![]( "Open in Gerrit")
int PseudoTcp::Recv(base::span<char> buffer) {Daniel AnguloCould this be uint8_t?
Done
if (!m_rbuf.Read(base::as_writable_bytes(buffer), &read)) {This can be removed after applying my previous comment.
Can we `CHECK_EQ(buffer->span().size(), buffer_size)`.
Eventually, the `buffer_size` argument would be removed, but this is going to take additional refactoring.
check_eq is throwing error because buffer->span() and buffer_size are not equal, have just added .first(buffer_size) to the Recv call
// SAFETY: buffer_size is the size of the buffer, and the buffer is a pointer
// to char of that size.We don't need a "safety" comment here. There are no UNSAFE_BUFERS
Done
int result = pseudo_tcp_.Recv(base::as_writable_chars(buffer->span()));This can be removed if you apply one of my other comments about passing `base::span<uint8_t>`
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 6 comments![]( "Open in Gerrit")
bool NotifyPacket(base::span<const char> buffer);Optional: Ma
int result = pseudo_tcp_.Recv(buffer->span().first(buffer_size));This could be `first` directly.
int result = pseudo_tcp_.Send(buffer->span().first(buffer_size));this could be `first`
// SAFETY: read_buffer_size_ is the size of the buffer, and the buffer is a
// pointer to char of that size.
int result = pseudo_tcp_.Recv(UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(read_buffer_->data()),
static_cast<size_t>(read_buffer_size_))));We could avoid `UNSAFE_BUFFERS`, here because `read_buffer_->first(read_buffer_size_)` exist.
(There is also `->span()` for the full span)
// SAFETY: write_buffer_size_ is the size of the buffer, and the buffer is a
// pointer to char of that size.
int result = pseudo_tcp_.Send(UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(write_buffer_->data()),
static_cast<size_t>(write_buffer_size_))));Ditto
pseudo_tcp_.NotifyPacket(UNSAFE_BUFFERS(Ditto
Related details
- Daniel Angulo
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 7 comments![]( "Open in Gerrit")
bool NotifyPacket(base::span<const char> buffer);Daniel AnguloOptional: Ma
Ma?
int result = pseudo_tcp_.Recv(buffer->span().first(buffer_size));This could be `first` directly.
Done
int result = pseudo_tcp_.Send(buffer->span().first(buffer_size));Daniel Angulothis could be `first`
Done
// SAFETY: read_buffer_size_ is the size of the buffer, and the buffer is a
// pointer to char of that size.
int result = pseudo_tcp_.Recv(UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(read_buffer_->data()),
static_cast<size_t>(read_buffer_size_))));We could avoid `UNSAFE_BUFFERS`, here because `read_buffer_->first(read_buffer_size_)` exist.
(There is also `->span()` for the full span)
Done
// SAFETY: write_buffer_size_ is the size of the buffer, and the buffer is a
// pointer to char of that size.
int result = pseudo_tcp_.Send(UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(write_buffer_->data()),
static_cast<size_t>(write_buffer_size_))));Daniel AnguloDitto
Done
pseudo_tcp_.NotifyPacket(UNSAFE_BUFFERS(Daniel AnguloDitto
Done
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 1 comment![]( "Open in Gerrit")
bool NotifyPacket(base::span<const char> buffer);Daniel AnguloOptional: Ma
Ma?
sorry, what did you say?
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 9 comments![]( "Open in Gerrit")
base::raw_span<const uint8_t> data;Do we know if there is any benchmarks around this code? This is the line most likely to trigger potential performance impact as we create and destroy segments.
void WriteString(const char* str, size_t len) {
// SAFETY: `str` is a raw char*, but we immediately convert it to a span of
// bytes, which makes our intentions clear and prevents buffer overflows.
auto bytes = base::as_byte_span(UNSAFE_BUFFERS(base::span(str, len)));There is no caller of this function? Lets just switch it to a span? or delete it.
*val = base::U32FromBigEndian(data_.subspan(pos_).first<4u>());Should we continue to use base::NetToHost32? Here and below to keep the convention?
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))Daniel AnguloWe can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
Stephen NuskoI was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
Daniel AnguloIf this function isn't used we should just delete it I think?
I guess the API it is copied from has one, so we could keep it.
Also I think std::string_view is fine? we are writing a string so accepting a string and converting it to bytes seems fine?
Stephen Nuskoisn't it writing data+pos into the same str that is the parameter?
Daniel AnguloObviously a char span works, but an std::string_view can be converted to a span cheaply so I'm not sure I follow the concern?
Since you already have to do base::as_writable_byte_span there is no difference but API clarity that you have a string? So better to use an std::string_view no?
base::as_writable_byte_span cannot be used with a std::string_view. Here's why
std::string_view is Read-Only: A std::string_view (and absl::string_view) provides a non-owning, read-only view into an existing character sequence. It doesn't own the data and cannot be used to modify it.
Ah right makes sense.
base::span<uint8_t> buffer(buffer_storage);Daniel AnguloYou don't need this variable, just pass `buffer_storage` directly into the SpanWriter.
And use the `subspan` method on `buffer_storage` directly for ReadOffset call.
no viable constructor or deduction guide for deduction of template arguments of 'base::SpanWriter'
589 | base::SpanWriter writer(buffer_storage);
I still prefer colapsing this to a single line:
```
base::SpanWriter writer(base::span(buffer_storage));
```
Also I think you might be able to do it with a template specification.
```
base::SpanWriter<uint8_t> writer(buffer_storage);
```
Which should remove one layer of guess work for the compiler.
Both seem more readable to me then a variable we don't use again
reader.Skip(1u);Daniel AnguloWhat is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
Stephen Nuskoin the writter we have buff[12] = 0; (which is the position 13) then buff[13] = flags (which is position 14). If I disable the skip then a bunch of tests start to fail
Daniel AnguloPerfect can you add a comment mentioning this? Makes sense.
Done
I'm sorry I don't see the comment?
Just something like.
```
reader.Skip(1u); // Skip the zero byte.
```
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
new_buffer.subspan(0, tail_copy)
.copy_from(buffer_.subspan(read_position_, tail_copy));
new_buffer.subspan(tail_copy, copy - tail_copy)
.copy_from(buffer_.subspan(0, copy - tail_copy));Daniel Angulocould this be written as a base::span::split? I think so?
Daniel AnguloDone
how? I see that there could be a split at tail_copy, but buffer_.subspan(0, copy - tail_copy) starts at offset 0 not at tail_copy, maybe I'm just a little bit confused but I don't see a clean split
I like this already better because the copy is clear what is going into what. But I think we can do the other as well.
```
auto new_buffer = base::HeapArray<uint8_t>::WithSize(size);
const size_t copy = data_length_;
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
// Split `new_buffer` into |:tail_copy:|:copy - tail_copy:| spans.
auto [new_buffer_first, new_buffer_last] =
new_buffer.as_span().first(copy).split_at(tail_copy);
// Split `buffer` into | buffer_.size() - read_position | read_position |
auto [before_read, after_read_position] =
buffer_.as_span().split_at(read_position_);
// Note: size > data_length_, but buffer_.size() >= data_length_ and unknown relation to size.
// Any value after read_position_ is the |tail_copy|, but before_read
// could be larger then the remaining |tail_copy - copy| so we adjust
// it to make them equal (old_buffer_end might be empty).
CHECK_EQ(old_buffer_start.size(), tail_copy);
old_buffer_end = old_buffer_end.first(tail_copy-copy);
new_buffer_first.copy_from_nonoverlapping(old_buffer_start);
new_buffer_last.copy_from_nonoverlapping(old_buffer_end);
```
Or something like that?
size_t tosend = std::min(static_cast<size_t>(kBlockSize),
send_buffer_.size() - send_stream_pos_);
if (tosend > 0) {
base::span(block).first(tosend).copy_from(base::as_bytes(
base::span(send_buffer_).subspan(send_stream_pos_, tosend)));
sent = local_.Send(base::span(block).first(tosend));We do this twice? and we never use "tosend" again. Perhaps rename it into a span and use it directly?
```suggestion
auto tosend = base::span(block).first(
std::min(static_cast<size_t>(kBlockSize),
send_buffer_.size() - send_stream_pos_));
if (!tosend.empty()) {
tosend.copy_from(base::as_bytes(
base::span(send_buffer_).subspan(send_stream_pos_, tosend.size())));
sent = local_.Send(tosend);
```
And consider this down below in other spots where it gets reused repeatedly?
int buffer_size,Daniel AnguloCan we `CHECK_EQ(buffer->span().size(), buffer_size)`.
Eventually, the `buffer_size` argument would be removed, but this is going to take additional refactoring.
check_eq is throwing error because buffer->span() and buffer_size are not equal, have just added .first(buffer_size) to the Recv call
Are there a lot of calls to it? Can we add the .first() to the callers of Read/Write? As Arthur said eventually we'd like to get rid of these, but if there are a lot of callers this might need to be an eventual cleanup.
It looks like each PseudoTcpAdapter::Core is only called from one spot, so could we just remove it and move the .first() into the PseudoTcpAdapter::Read/Write? although it also appears they are only called in a couple locations (like 4 files or so each), so we could move the .first() even higher there.
If you'd like to keep the CL from exploding could we at least add the CHECK arthur requested here, and update PseudoTcpAdapter::Read/Write to call .first() there? And then as a follow up you could propagate it up one level to the 4-8 files.
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
bool NotifyPacket(base::span<const char> buffer);Daniel AnguloOptional: Ma
Daniel AnguloMa?
sorry, what did you say?
Optional: Maybe this could be `“ase::span<const uint8_t>` directly. This would avoid conversion internally and avoid mixing the two types in this class.
Of course, that would move the need to convert to the callers, but I think this is better.
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |