Fix a issue with initializing blob header buffer #8537
Conversation
zaorangyang
changed the title
| @@ -69,7 +71,8 @@ class BlobLogSequentialReader { | |||
| Statistics* statistics_; | |||
|
|
|||
| Slice buffer_; | |||
| char header_buf_[BlobLogRecord::kHeaderSize]; | |||
| char header_buf_[MAX_HEADER_SIZE(BlobLogHeader::kSize, BlobLogFooter::kSize, | |||
There was a problem hiding this comment.
Technically, BlobLogRecord::kHeaderSize is large enough to cover all three, see also the static_asserts here:
https://github.com/facebook/rocksdb/blob/master/db/blob/blob_log_sequential_reader.cc#L48-L49
https://github.com/facebook/rocksdb/blob/master/db/blob/blob_log_sequential_reader.cc#L67-L68
https://github.com/facebook/rocksdb/blob/master/db/blob/blob_log_sequential_reader.cc#L117-L118
Having said that, I think this is a good change from a theoretical standpoint.
| @@ -10,6 +10,8 @@ | |||
| #include "db/blob/blob_log_format.h" | |||
| #include "rocksdb/slice.h" | |||
|
|
|||
| #define MAX_HEADER_SIZE(a, b, c) (a > b ? (a > c ? a : c) : (b > c ? b : c)) | |||
There was a problem hiding this comment.
Would be nice to use std::max here but it is unfortunately constexpr only since C++14.
There was a problem hiding this comment.
Could you #undef the macro near the bottom of the file just in case?
|
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@zaorangyang has updated the pull request. You must reimport the pull request before landing. |
|
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@jay-zhuang merged this pull request in 8e91bd9. |
No description provided.