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

add support for binary data read for Table::get() #836

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

Yakiv-Huryk
Copy link
Contributor

In the current implementation, the data from redis is passed directly to the std::string(char*) constructor which truncates the data on the null byte.
To support binary data that can contain null bytes, the redis reply str is passed along with its len into the std::string(char*,size_t) constructor that supports the input size.

In the current implementation, the data from redis is passed directly
to the std::string(char*) constructor which truncates the data on the null
byte.
To support binary data that can contain null bytes, the redis reply
*str* is passed along with its *len* into the std::string(char*,size_t)
constructor that supports the input size.
@Pterosaur
Copy link
Contributor

Hi @liuh-80 , please help check and merge this PR.

@qiluo-msft qiluo-msft merged commit 5d1fe2d into sonic-net:master Dec 4, 2023
14 checks passed
@liuh-80
Copy link
Contributor

liuh-80 commented Dec 5, 2023

@Yakiv-Huryk , I found this PR will cause build break in sonic-buildimage, please check and fix:

https://dev.azure.com/mssonic/be1b070f-be15-4154-aade-b1d3bfb17054/_apis/build/builds/426203/logs/58

In constructor 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(const _CharT*, size_type, const _Alloc&) [with _CharT = char; _Traits = std::char_traits; _Alloc = std::allocator]',
inlined from 'virtual void Table_binary_data_get_Test::TestBody()' at tests/redis_ut.cpp:856:53:
/usr/include/c++/12/bits/basic_string.h:620:21: error: array subscript 8 is outside array bounds of 'const char [6]' [-Werror=array-bounds]
620 | _M_construct(__s, __s + __n, std::forward_iterator_tag());
| ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In constructor 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(const _CharT*, size_type, const _Alloc&) [with _CharT = char; _Traits = std::char_traits; _Alloc = std::allocator]',
inlined from 'virtual void Table_binary_data_get_Test::TestBody()' at tests/redis_ut.cpp:857:53:
/usr/include/c++/12/bits/basic_string.h:620:21: error: array subscript 8 is outside array bounds of 'const char [6]' [-Werror=array-bounds]
620 | _M_construct(__s, __s + __n, std::forward_iterator_tag());
| ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@Yakiv-Huryk
Copy link
Contributor Author

@Yakiv-Huryk , I found this PR will cause build break in sonic-buildimage, please check and fix:

Fixed in the #841

@Pterosaur
Copy link
Contributor

@yxieca Could you please approve this PR to 202311 branch? Because The PR #878 PR depends on this.

mssonicbld pushed a commit to mssonicbld/sonic-swss-common that referenced this pull request May 29, 2024
In the current implementation, the data from redis is passed directly
to the std::string(char*) constructor which truncates the data on the null
byte.
To support binary data that can contain null bytes, the redis reply
*str* is passed along with its *len* into the std::string(char*,size_t)
constructor that supports the input size.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202311: #880

Pterosaur added a commit that referenced this pull request May 30, 2024
…836)

In the current implementation, the data from redis is passed directly
to the std::string(char*) constructor which truncates the data on the null
byte.
To support binary data that can contain null bytes, the redis reply
*str* is passed along with its *len* into the std::string(char*,size_t)
constructor that supports the input size.

Co-authored-by: Yakiv Huryk <[email protected]>
Co-authored-by: Ze Gan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants