Add flatbuffer support for COM configuration using flatbuffers#66
Conversation
Signed-off-by: KaitoKid <thomas.mikhael@icloud.com>
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested changes to align SOME/IP handling, tighten logging/security, and add license header.
Status: Changes Suggested | Risk: Medium | Readiness: 6/10 (Needs Polish)
Issues Identified & Suggestions
- Align SOME/IP handling to skip unsupported bindings, avoid startup crash:
score/mw/com/impl/configuration/flatbuffer_config_loader.cpp - Reduce path logging and use thread-safe errno handling:
score/mw/com/impl/configuration/flatbuffer_config_loader.cpp - Add Apache-2.0 SPDX header to new tool script:
bazel/tools/json_to_flatbuffer_json.py
Review Details
📁 20 files reviewed | 💬 3 comments
👍 / 👎 individual comments to help improve reviews for you
| else if (binding->binding() == BindingType::SOME_IP) | ||
| { | ||
| // Skip SOME/IP - not supported yet | ||
| ::score::mw::log::LogFatal("lola") << "Provided SOME/IP binding, which is not supported yet."; | ||
| std::terminate(); | ||
| } | ||
| else | ||
| { | ||
| ::score::mw::log::LogFatal("lola") << "Unknown binding type provided. Required argument."; |
There was a problem hiding this comment.
[Logic] In CreateServiceTypes() (lines 367-372), the FlatBuffer loader terminates when a SOME/IP binding is encountered:
else if (binding->binding() == BindingType::SOME_IP)
{
// Skip SOME/IP - not supported yet
::score::mw::log::LogFatal("lola") << "Provided SOME/IP binding, which is not supported yet.";
std::terminate();
}Note the contradictory comment: it says "Skip SOME/IP" but then terminates.
In the existing JSON loader (config_parser.cpp, ParseServiceTypeDeployment at lines 933-936), SOME/IP bindings in service types are silently skipped so that the loop continues scanning for an SHM binding:
else if (value == kSomeIpBinding)
{
// we skip this, because we don't support SOME/IP right now.
}This means any configuration that lists a SOME/IP binding before an SHM binding in service_types will crash during startup when using the FlatBuffer loader, while it works fine with the JSON parser.
Please align the FlatBuffer loader's behavior: replace the LogFatal + std::terminate() with a continue (or log a warning and continue) so that SOME/IP bindings are skipped and the loop proceeds to find the SHM binding.
Note: The SOME/IP handling in CreateServiceInstances() (lines 444-448) is consistent with the JSON parser, which also terminates there — no change needed for service instances.
Context for Agents
In `CreateServiceTypes()` (lines 367-372), the FlatBuffer loader terminates when a SOME/IP binding is encountered:
```cpp
else if (binding->binding() == BindingType::SOME_IP)
{
// Skip SOME/IP - not supported yet
::score::mw::log::LogFatal("lola") << "Provided SOME/IP binding, which is not supported yet.";
std::terminate();
}
```
Note the contradictory comment: it says "Skip SOME/IP" but then terminates.
In the existing JSON loader (`config_parser.cpp`, `ParseServiceTypeDeployment` at lines 933-936), SOME/IP bindings in service types are silently skipped so that the loop continues scanning for an SHM binding:
```cpp
else if (value == kSomeIpBinding)
{
// we skip this, because we don't support SOME/IP right now.
}
```
This means any configuration that lists a SOME/IP binding before an SHM binding in `service_types` will crash during startup when using the FlatBuffer loader, while it works fine with the JSON parser.
Please align the FlatBuffer loader's behavior: replace the `LogFatal` + `std::terminate()` with a `continue` (or log a warning and continue) so that SOME/IP bindings are skipped and the loop proceeds to find the SHM binding.
Note: The SOME/IP handling in `CreateServiceInstances()` (lines 444-448) is consistent with the JSON parser, which also terminates there — no change needed for service instances.
File: score/mw/com/impl/configuration/flatbuffer_config_loader.cpp
Line: 375Chain of Thought
- Validation Decision: accepted
- Confidence: 0.92
- Likelihood Score: 4/5
- Likelihood Reason: If any configuration declares both SOME/IP and SHM bindings in service_types and the SOME/IP entry appears before SHM, the FlatBuffer loader will crash at startup. This is a realistic scenario for configs that were originally designed for multi-binding or that simply list both binding types. The JSON parser handles this gracefully by skipping SOME/IP.
- Suggested Action: modify
- Tool Calls Used: 11
Reasoning
The comment correctly identifies a behavioral regression between the JSON parser and the new FlatBuffer loader for service type binding parsing.
Evidence from JSON parser (config_parser.cpp, ParseServiceTypeDeployment, lines 919-952):
else if (value == kSomeIpBinding)
{
// we skip this, because we don't support SOME/IP right now.
}SOME/IP bindings are silently skipped, and the loop continues to find an SHM binding.
Evidence from FlatBuffer loader (flatbuffer_config_loader.cpp, CreateServiceTypes, lines 367-372):
else if (binding->binding() == BindingType::SOME_IP)
{
// Skip SOME/IP - not supported yet
::score::mw::log::LogFatal("lola") << "Provided SOME/IP binding, which is not supported yet.";
std::terminate();
}The code even has a comment saying "Skip SOME/IP" but then terminates instead of skipping!
This is a genuine behavioral regression introduced by this PR (the entire file is new). Any configuration that lists a SOME/IP binding before an SHM binding in its service_types section will crash at startup with the FlatBuffer loader, while it works fine with the JSON parser.
Note: For service instances (CreateServiceInstances), BOTH the JSON parser (line 640-644) and the FlatBuffer loader (lines 444-448) terminate on SOME/IP, so they are consistent there. The regression is specifically in the service type deployment parsing.
The fix suggestion is clear and actionable: remove the LogFatal + std::terminate() for SOME/IP in CreateServiceTypes() and replace with a continue (or just empty block) to skip it, matching the JSON parser's behavior.
Evidence Summary
1. File Lookup
- Filename:
config_parser.cpp
Found 2 file(s) matching 'config_parser.cpp':
- score/mw/com/impl/configuration/config_parser.cpp
- score/mw/com/performance_benchmarks/macro_benchmark/config_parser.cpp
2. File Snapshot (BASE)
- File:
score/mw/com/impl/configuration/config_parser.cpp - Lines:
300-420
300: // coverity[autosar_cpp14_a15_5_3_violation]
301: std::string GetName(const score::json::Object::const_iterator name) const noexcept
302: {
303: if (name == json_object_.cend())
304: {
305: score::mw::log::LogFatal("lola") << "No Event/Field-Name provided. Required attribute";
306: /* Terminate call tolerated.See Assumptions of Use in mw/com/design/README.md*/
307: std::terminate();
308: }
309: return name->second.As<std::string>().value().get();
...
3. File Snapshot (BASE)
- File:
score/mw/com/impl/configuration/config_parser.cpp - Lines:
1-100
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Snapshot (BASE)
- File:
score/mw/com/impl/configuration/config_parser.cpp - Lines:
100-200
100: constexpr auto kSomeIpBinding = "SOME/IP"sv;
101: constexpr auto kShmBinding = "SHM"sv;
102: constexpr auto kShmSizeCalcModeSimulation = "SIMULATION"sv;
103:
104: constexpr auto kTracingTraceFilterConfigPathDefaultValue = "./etc/mw_com_trace_filter.json"sv;
105: constexpr auto kStrictPermission = "strict"sv;
106: constexpr auto kFilePermissionsOnEmpty = "file-permissions-on-empty"sv;
107:
108: void ErrorIfFound(const score::json::Object::const_iterator& iterator_to_element, const score::json::Object& json_obj)
109: {
...
5. File Snapshot (BASE)
- File:
score/mw/com/impl/configuration/config_parser.cpp - Lines:
200-300
200: return QualityType::kASIL_QM;
201: }
202:
203: if (qualityValue == "B")
204: {
205: return QualityType::kASIL_B;
206: }
207:
208: return QualityType::kInvalid;
209: }
...
6. File Snapshot (BASE)
- File:
score/mw/com/impl/configuration/config_parser.cpp - Lines:
700-900
700:
701: auto instance_deployments = ParseServiceInstanceDeployments(
702: serviceInstance, tracing_configuration, service_identifier, instanceSpecifier);
703: if (instance_deployments.size() != 1U)
704: {
705: score::mw::log::LogFatal("lola") << "More or less then one deployment for " << service_identifier.ToString()
706: << ". Multi-Binding support right now not supported";
707: /* Terminate call tolerated.See Assumptions of Use in mw/com/design/README.md*/
708: std::terminate();
709: }
...
7. File Snapshot (BASE)
- File:
score/mw/com/impl/configuration/config_parser.cpp - Lines:
500-700
500:
501: // See Note 1
502: // coverity[autosar_cpp14_a15_5_3_violation]
503: auto ParseServiceElementTracingEnabled(const score::json::Any& json,
504: TracingConfiguration& tracing_configuration,
505: const std::string_view service_type_name_view,
506: const InstanceSpecifier& instance_specifier,
507: const ServiceElementType service_element_type) noexcept
508: {
509: SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(
...
8. File Snapshot (BASE)
- File:
score/mw/com/impl/configuration/config_parser.cpp - Lines:
900-1050
900: }
901: if (!AreEventFieldAndMethodIdsUnique(lola))
902: {
903: score::mw::log::LogFatal("lola") << "Configuration cannot contain duplicate eventId, fieldId, or methodId.";
904: /* Terminate call tolerated.See Assumptions of Use in mw/com/design/README.md*/
905: std::terminate();
906: }
907: return lola;
908: }
909: else
...
|
|
||
| // Map file into memory | ||
| void* ptr = mmap(nullptr, mapped_size, PROT_READ, MAP_PRIVATE, fd_, 0); | ||
| if (ptr == MAP_FAILED) |
There was a problem hiding this comment.
[Security] Information leak in logs: config_loader.cpp logs the full file path via LogInfo. In flatbuffer_config_loader.cpp, LoadBuffer logs paths and raw strerror(errno) messages via LogFatal. While these are fatal paths, the LogInfo calls on every startup expose filesystem layout to log consumers. Consider whether this level of detail is appropriate for production logs.
More importantly, std::strerror(errno) is not thread-safe on all platforms. Use a thread-safe alternative or copy errno immediately.
Context for Agents
Information leak in logs: `config_loader.cpp` logs the full file path via `LogInfo`. In `flatbuffer_config_loader.cpp`, `LoadBuffer` logs paths and raw `strerror(errno)` messages via `LogFatal`. While these are fatal paths, the `LogInfo` calls on every startup expose filesystem layout to log consumers. Consider whether this level of detail is appropriate for production logs.
More importantly, `std::strerror(errno)` is not thread-safe on all platforms. Use a thread-safe alternative or copy `errno` immediately.
File: score/mw/com/impl/configuration/flatbuffer_config_loader.cpp
Line: 275Chain of Thought
- Validation Decision: accepted
- Confidence: 0.00
- Likelihood Score: 0/5
- Suggested Action: keep
- Tool Calls Used: 10
Reasoning
Failed to parse validation response
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/configuration/config_loader.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| @@ -0,0 +1,124 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
[Company Standards] The new tool script is missing the standard Apache 2.0 copyright/SPDX header that other repository files include. Add the header block (after the shebang) to keep license compliance consistent.
Context for Agents
The new tool script is missing the standard Apache 2.0 copyright/SPDX header that other repository files include. Add the header block (after the shebang) to keep license compliance consistent.
File: bazel/tools/json_to_flatbuffer_json.py
Line: 1Chain of Thought
- Validation Decision: accepted
- Confidence: 0.88
- Likelihood Score: 4/5
- Likelihood Reason: The file is newly introduced in this PR and is clearly missing the Apache 2.0 copyright/SPDX header that C++ files in the same PR consistently include. This will cause license compliance inconsistency.
- Suggested Action: keep
- Tool Calls Used: 13
Reasoning
The comment claims the new Python script bazel/tools/json_to_flatbuffer_json.py is missing the standard Apache 2.0 copyright/SPDX header.
Evidence gathered:
- The file is entirely new (added in this PR), starting with
#!/usr/bin/env python3followed by a docstring with no copyright header. - C++ files in the same PR (e.g.,
flatbuffer_config_loader.cpp,config_loader.cpp) consistently include the standard copyright header block with "Copyright (c) 2025 Contributors to the Eclipse Foundation", Apache License 2.0 reference, andSPDX-License-Identifier: Apache-2.0. - The issue is clearly introduced by this PR since the file is new.
- The comment is actionable: it specifies exactly what to add (the Apache 2.0 copyright/SPDX header) and where (after the shebang line).
Note: The two .bzl files (generate_com_config.bzl, validate_flatbuffer_json.bzl) are also new files in this PR and similarly lack the copyright header, which further supports that this is an oversight pattern in the PR. The comment is valid and points to a real consistency issue.
Evidence Summary
1. File Snapshot (HEAD)
- File:
bazel/tools/generate_com_config.bzl - Lines:
1-20
1: """
2: Bazel rule to generate FlatBuffer binary configuration from JSON files.
3:
4: This rule converts existing communication JSON files to a FlatBuffer friendly format.
5: - Convert '-' to '_' in keys (required)
6: - Convert keys from camelCase to snake_case (avoids warnings)
7: """
8:
9: def generate_com_config(name, json, convert, visibility = None):
10: """
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/configuration/flatbuffer_config_loader.cpp - Lines:
1-20
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Snapshot (HEAD)
- File:
score/mw/com/impl/configuration/config_loader.cpp - Lines:
1-20
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Snapshot (HEAD)
- File:
bazel/tools/validate_flatbuffer_json.bzl - Lines:
1-20
1: """
2: Validate a JSON file against a FlatBuffer schema.
3:
4: This rule converts existing communication JSON files to a FlatBuffer friendly format.
5: - Convert '-' to '_' in keys (required)
6: - Convert keys from camelCase to snake_case (avoids warnings)
7: """
8:
9: def _validate_json_flatbuffer_test_impl(ctx):
10: """Implementation of the validation test rule."""
...
5. File Lookup
- Filename:
.py - Max Results:
10
No files found matching '.py'
3 participants
resolves eclipse-score#110
This initial version was focused on working ipc bridge test using flatbuffers, quality aspects are not yet covered.
It is recommended to break down the issue into smaller tasks and setup a merge strategy, PR is therefore draft only.
Pending decisions:
Changes (rough outline)
Open points:
This also formalizes the COM configuration structure with a dedicated FlatBuffer schema and integrates build-time automation to emit FlatBuffer binaries from existing JSON, positioning the format change as groundwork for future performance gains.
Architecture Diagram:
flowchart TD subgraph Build_Layer B["Bazel Build System (+9 lines)"] P[/"Python Converter (+124 lines)"/] G["generate_com_config (Bazel Rule +100 lines)"] V["validate_json_flatbuffer_test (Bazel Rule +128 lines)"] end subgraph Configuration_Layer J[/"JSON Config Files"/] F["FlatBuffer Schema (.fbs +157 lines)"] FB[/"FlatBuffer Binary Files"/] end subgraph Runtime_Layer CL["Config Loader (+57 lines)"] FCL["FlatBufferConfigLoader (+539 lines)"] CP["Config Parser"] RC["Runtime Configuration (+25 lines)"] R["Runtime (+7 lines)"] end J -->|converted by| P P -->|outputs| J_FB[/"FlatBuffer-friendly JSON"/] J_FB -->|compiled by| G F -->|used by| G G -->|generates| FB J_FB -->|validated by| V F -->|used by| V RC -->|requests config from| CL CL -->|loads .json via| CP CL -->|loads .bin via| FCL CP -->|parses| J FCL -->|reads & parses| FB R -->|uses| RC R -->|uses| CLThis summary was automatically generated by @propel-code-bot