feat: Introduce serializer "plugin" interface and null_serializer#129
feat: Introduce serializer "plugin" interface and null_serializer#129NEOatNHNG wants to merge 10 commits into
Conversation
TODO: actually use serializer
|
The created documentation from the pull request is available at: docu-html |
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
mariuswbr
left a comment
There was a problem hiding this comment.
Had an initial look over the changes.
|
|
||
| union SerializationConfig { | ||
| NullSerializerConfig, | ||
| } |
There was a problem hiding this comment.
So the idea is then for an actual serializer configuration to add it to the union?
Shouldn't we define the table for this already as well? Or would that be done with the actual serializer implementation?
Maybe leave a comment/hint then?
| cc_library( | ||
| name = "null_serializer_impl", | ||
| srcs = ["null_serializer.cpp"], | ||
| deps = [ | ||
| ":interface", | ||
| ":pre_serialized_data", | ||
| "//src/config:config_flatbuffers", | ||
| ], | ||
| ) | ||
|
|
||
| # Serializer that can only handle pre-serialized data and is used as default serializer for gatewayd if no other serializer is provided. | ||
| cc_shared_library( | ||
| name = "null_serializer", | ||
| shared_lib_name = "score_com_serializer.so", | ||
| visibility = ["//visibility:public"], | ||
| deps = [":null_serializer_impl"], | ||
| ) |
There was a problem hiding this comment.
Can't this just be combined to a single cc_shared_library?
I'm becoming less and less of a fan of too many bazel targets. It gets annoying quickly if you have to list too many deps
| /// Retrieves the maximum serialized size of the serializer. | ||
| /// @param serializer Pointer to the serializer | ||
| std::size_t score_com_serializer_get_max_serialized_size( | ||
| const struct score_com_serializer* serializer); | ||
| /// Retrieves the sizeof() of the C++ data type that the serializer handles. | ||
| /// @param serializer Pointer to the serializer | ||
| /// @retval 0 if not specified by the serializer. Deserialization might not work. | ||
| std::size_t score_com_serializer_get_sizeof_object(const struct score_com_serializer* serializer); | ||
| /// Retrieves the alignof() of the C++ data type that the serializer handles. | ||
| /// @param serializer Pointer to the serializer | ||
| /// @retval 0 if not specified by the serializer. Deserialization might not work. | ||
| std::size_t score_com_serializer_get_alignof_object(const struct score_com_serializer* serializer); |
There was a problem hiding this comment.
Can you add an empty line in between the functions and the comments for the next function?
|
|
||
| /// Retrieves a serializer for the specified interface and component. | ||
| /// @param service_type Identifies the interface for which the serializer is requested. | ||
| /// @param service_type_size Size of the `service_type` string in bytes. |
There was a problem hiding this comment.
Including or excluding null termination? Same thing for the element_name_size.
| const struct score_com_serializer* serializer, const uint8_t* buffer, size_t buffer_size, | ||
| void* object); | ||
|
|
||
| /// Retrieves the maximum serialized size of the serializer. |
There was a problem hiding this comment.
Sounds a bit weird. Could be mis-read?
Maybe: "Retrieves the maximum serialized size from the serializer."
Or: "Retrieves the maximum size of the C++ data type after serialization that the serializer handles."
| /// Initializes the serializer plugin. This function must be called once before any calls to | ||
| /// score_com_serializer_get(). Not thread-safe. | ||
| /// @param serializer_identifier Pointer to a string that identifies the serializer plugin. | ||
| /// @param serializer_identifier_size Size of the serializer identifier string in bytes. |
There was a problem hiding this comment.
Including or excluding null termination?
| if (buffer == nullptr || object == nullptr) { | ||
| return score_com_serializer_result_general_failure; | ||
| } | ||
| const auto* pre_serialized_data = static_cast<const PreSerializedData<0>*>(object); |
There was a problem hiding this comment.
Took me a minute to understand why the cast to PreSerializedData with MaxMessageSize = 0.
Can we add something like using PreSerializedDataView = PreSerializedData<0>; and give some explanation in pre_serialized_data.h?
Explanation maybe: "View alias for accessing PreSerializedData fields when MaxMessageSize is unknown at compile time."
Or do you think it's clear enough?
lurtz
left a comment
There was a problem hiding this comment.
I would appreciate a markdown or rst file explaining the high level picture in up to 10 sentences.
| if (score_com_serializer_deinit() != score_com_serializer_result_ok) { | ||
| std::cerr << "Warning: Failed to deinitialize serializer plugin." << std::endl; | ||
| } |
There was a problem hiding this comment.
We are doing C++ and can make sure via destructors cleanup is always done. socom already has a Final_action class, which you may use. Or there is even something in baselibs
| assert(pre_serialized_response_sample->size == sizeof(EchoResponseTiny)); | ||
| auto* response_sample = | ||
| reinterpret_cast<const EchoResponseTiny*>(pre_serialized_response_sample->data); |
There was a problem hiding this comment.
I see this pattern repeated a few times. Does it make sense to extract it into a function? Then there is only one place which asserts and reinterpret_casts.
| #include <cstddef> // Required for size_t | ||
| #include <cstdint> |
There was a problem hiding this comment.
if you want a C interface you should also use proper C headers at the includes
| #endif | ||
|
|
||
| /// Result type for the serializer functions | ||
| enum [[nodiscard]] score_com_serializer_result { |
There was a problem hiding this comment.
Is [[attr]] understood by C parsers?
| /// @param serializer Pointer to the serializer | ||
| std::size_t score_com_serializer_get_max_serialized_size( | ||
| const struct score_com_serializer* serializer); | ||
| /// Retrieves the sizeof() of the C++ data type that the serializer handles. |
There was a problem hiding this comment.
is this the size of the internal buffer? Or is this the payload size into which the object gets serialized into? I guess the comment needs refinement
| /// Type definition for pre-serialized data which is used by an application to provide | ||
| /// pre-serialized data to gatewayd. The serializer can then be skipped. | ||
| template <std::size_t MaxMessageSize> | ||
| struct PreSerializedData { |
There was a problem hiding this comment.
I am confused. How does the serializer know it is dealing with an object of type PreSerializedData? I also thought that the NullSerializer would be used in this case.
| score_com_serializer_result score_com_serializer_get( | ||
| const char* service_type, size_t service_type_size, | ||
| enum score_com_serializer_element_type element_type, const char* element_name, | ||
| size_t element_name_size, const struct score_com_serializer** serializer) { |
There was a problem hiding this comment.
Does this mean that only one serializer type per gatewayd is possible? If yes that is a design decision which should be documented, if it is not already.
| } | ||
|
|
||
| // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) | ||
| *serializer = reinterpret_cast<const struct score_com_serializer*>(serializer_config); |
There was a problem hiding this comment.
Does it make more sense to write this
typedef NullSerializerConfig score_com_serializer;
Then you do not need to reinterpret_cast. However I assume this might cause havoc if we decide to have different serializer types running in the same process.
|
|
||
| EXPECT_EQ(result, score_com_serializer_result_ok); | ||
| EXPECT_EQ(written_bytes, sizeof(test_data)); | ||
| EXPECT_EQ(std::memcmp(buffer.data(), test_data, sizeof(test_data)), 0); |
There was a problem hiding this comment.
is std::equal easier to understand than memcmp?
Improvement
Description
Add serializer interface and a null_serializer (which can handle pre-serialized messages) to the gatewayd.
Related ticket
closes #10