net: rpc: openthread: dynamic allocation for CLI commands #19320
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
changelog-entry-required
bpienk
approved these changes
Dec 6, 2024
canisLupus1313
approved these changes
Dec 6, 2024
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 93d84930d87841bdcb4d3f8148d23181a7fc47ad more detailssdk-nrf:
Github labels
List of changed files detected by CI (3)Outputs:ToolchainVersion: b77d8c1312 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
kderda
force-pushed
the
rpc_cli_malloc
branch
from
e76b1ef to
e540759
Compare
|
You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds. Note: This comment is automatically posted by the Documentation Publish GitHub Action. |
kderda
force-pushed
the
rpc_cli_malloc
branch
from
e540759 to
e9c4460
Compare
lmaciejonczyk
approved these changes
Dec 19, 2024
anangl
reviewed
Dec 19, 2024
| @@ -232,7 +232,7 @@ char *nrf_rpc_decode_str(struct nrf_rpc_cbor_ctx *ctx, char *buffer, size_t buff | |||
| /** @brief Decode a string pointer and length. Moves CBOR pointer past string on success. | |||
| * | |||
| * @param[in,out] ctx CBOR decoding context. | |||
| * @param[out] size String length. | |||
| * @param[out] len String length. | |||
There was a problem hiding this comment.
This should be corrected in the first commit that introduces nrf_rpc_decode_str_ptr_and_len(), not here where dynamic allocation is added.
Comment on lines
79
to
91
| if (ptr) { | ||
| buffer = malloc(len + 1); | ||
| if (buffer) { | ||
| memcpy(buffer, ptr, len); | ||
| buffer[len] = '\0'; | ||
| } | ||
| } | ||
|
|
||
| if (!nrf_rpc_decoding_done_and_check(group, ctx)) { | ||
| ot_rpc_report_cmd_decoding_error(OT_RPC_CMD_CLI_INPUT_LINE); | ||
| return; |
There was a problem hiding this comment.
The function can return here without freeing buffer. Is this intended?
kderda
force-pushed
the
rpc_cli_malloc
branch
from
e9c4460 to
59693b0
Compare
anangl
approved these changes
Dec 19, 2024
| memcpy(buffer, ptr, len); | ||
| buffer[len] = '\0'; | ||
| } | ||
| } | ||
|
|
||
| if (!nrf_rpc_decoding_done_and_check(group, ctx)) { |
There was a problem hiding this comment.
Why not move this before the allocation and copying which turn out to be unnecessary when this fails? This extra call to free would not be needed then.
There was a problem hiding this comment.
nrf_rpc_decoding_done_and_check() frees the buffer which holds the data to be stored in the buffer.
This commit adds new helper function nrf_rpc_decode_str_ptr_and_len() that decodes CBOR string without copying it to a buffer. It allows to get string length before buffer allocation. Signed-off-by: Konrad Derda <[email protected]>
CLI commands may require large buffer for command strings. This commit introduces usage of dynamic allocation to avoid usage of huge static buffer. Signed-off-by: Konrad Derda <[email protected]>
kderda
force-pushed
the
rpc_cli_malloc
branch
from
59693b0 to
93d8493
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.