modify return value

[jishengruofou]

modify return value.

Summary by CodeRabbit

• Bug Fixes

  • Sample flow now returns the actual outcome of the last operation instead of always reporting success, improving error visibility.

• Improvements

  • Read/write operations now report success/failure more accurately, enabling clearer error propagation.
  • Initialization order refined to avoid unnecessary work on allocation failure, improving robustness and preventing misleading state.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The GSM CMUX sample now returns the last CMUX operation result instead of always returning success; CMUX core functions changed send/read/write return types from rt_size_t to rt_ssize_t, and initialization memset was reordered after allocation checks.

Changes

Cohort / File(s)	Summary
Sample: return propagation sample/cmux_sample_gsm.c	End label changed to return result; so the sample propagates the final CMUX operation result instead of returning a constant success value.
Core: API return type & init reorder src/cmux.c	Updated cmux_send_data, cmux_vcom_write, and cmux_vcom_read return types from rt_size_t to rt_ssize_t; changed local len to rt_ssize_t. Moved memset(object->vcoms, ...) to after vcom allocation/null-check; minor formatting tweaks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'modify return value' is vague and generic, using non-descriptive language that doesn't convey meaningful information about the specific changes.	Provide a more specific title that identifies which functions or modules are affected, such as 'Change cmux function return types from rt_size_t to rt_ssize_t' or 'Update return value handling in cmux sample and core functions'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 125f48d and 9836841.

📒 Files selected for processing (1)

• src/cmux.c

🔇 Additional comments (4)

src/cmux.c (4)

80-80: LGTM!

The return type change from rt_size_t to rt_ssize_t is appropriate for supporting error codes.

---

652-658: Excellent fix: memset moved after allocation check.

Moving memset to line 658 (after the null check at line 653) correctly prevents a potential null pointer dereference if the allocation fails.

---

834-847: LGTM! Type changes are consistent.

The return type and local variable len type changes from rt_size_t to rt_ssize_t are consistent with the cmux_send_data return type change. However, ensure that the error handling in cmux_send_data is corrected to return negative error codes (see previous comment).

---

859-929: LGTM! Return type change is appropriate.

The return type change from rt_size_t to rt_ssize_t is forward-compatible and allows for future error handling improvements with negative error codes.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @src/cmux.c:
- Line 530: The declaration and definition of cmux_send_data disagree on the
return type; update the function prototype for cmux_send_data to return
rt_ssize_t (matching the definition) so the declaration signature static
rt_ssize_t cmux_send_data(struct rt_device *dev, int port, rt_uint8_t type,
const char *data, int length); matches the implementation and recompile to
verify the warning is resolved.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a47fe69 and b35b6f6.

📒 Files selected for processing (1)

• src/cmux.c

🔇 Additional comments (1)

src/cmux.c (1)

651-658: LGTM! Correct fix for memory initialization.

The memset now properly executes only after verifying that the allocation succeeded, preventing undefined behavior when rt_malloc returns NULL. This addresses the concern mentioned in the commit message about fixing memory operations when allocation fails.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/cmux.c (2)

914-916: Redundant check with potential logic flaw.

The condition if (vcom->frame->data_length - vcom->length >= 0) is always true when both operands are unsigned types (which they appear to be). This means:

1. The check provides no actual protection against underflow.
2. If the condition were somehow false, read_len would be returned uninitialized at line 918.

🔧 Proposed fix

Replace with a proper bounds check or remove the condition entirely:

-            if (vcom->frame->data_length - vcom->length >= 0) {
-                read_len = vcom->frame->data_length - vcom->length;
-            }
+            read_len = vcom->frame->data_length - vcom->length;

Or, if guarding against underflow is intended:

+            size_t read_len = 0;
+            if (vcom->frame->data_length >= vcom->length) {
+                read_len = vcom->frame->data_length - vcom->length;
+            }
-            if (vcom->frame->data_length - vcom->length >= 0) {
-                read_len = vcom->frame->data_length - vcom->length;
-            }

---

834-847: Update forward declaration at line 80 to match function definition.

The forward declaration declares cmux_send_data as returning rt_size_t (line 80), but the definition at line 530 returns rt_ssize_t. This signature mismatch must be fixed:

Location

Line 80:

static rt_size_t cmux_send_data(struct rt_device *dev, int port, rt_uint8_t type, const char *data, int length);

Should be:

static rt_ssize_t cmux_send_data(struct rt_device *dev, int port, rt_uint8_t type, const char *data, int length);

Additionally, note that cmux_send_data returns 0 on error (lines 557, 563, 569), not negative error codes. The change to rt_ssize_t doesn't provide better error propagation unless the function is updated to return negative values on failure.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b35b6f6 and 125f48d.

📒 Files selected for processing (1)

• src/cmux.c

🔇 Additional comments (2)

src/cmux.c (2)

652-658: Good defensive programming.

Moving rt_memset after the allocation check ensures we don't attempt to zero-initialize a NULL pointer. This correctly prevents a potential crash if rt_malloc fails.

---

859-862: No action needed—signature is correct.

The function signature matches RT-Thread's device API contract. Modern RT-Thread versions (v5.0.0+) expect read/write operations in rt_device_ops to return rt_ssize_t, not rt_size_t. The implementation is compliant with the framework's API specification.

Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/cmux.c (1)

530-580: Return negative error codes instead of 0.

The return type was changed to rt_ssize_t (signed), but error conditions still return 0 (lines 559, 567, 574). With signed return types, the convention is to return negative error codes to distinguish errors from successful zero-length writes.

🔧 Proposed fix to return proper error codes

     c = rt_device_write(dev, 0, prefix, prefix_length);
     if (c != prefix_length)
     {
         LOG_E("Couldn't write the whole prefix to the serial port for the virtual port %d. Wrote only %d  bytes.", port, c);
-        return 0;
+        return -RT_EIO;
     }
     if (length > 0)
     {
         c = rt_device_write(dev, 0, data, length);
         if (length != c)
         {
             LOG_E("Couldn't write all data to the serial port from the virtual port %d. Wrote only %d bytes.", port, c);
-            return 0;
+            return -RT_EIO;
         }
     }
     c = rt_device_write(dev, 0, postfix, 2);
     if (c != 2)
     {
         LOG_E("Couldn't write the whole postfix to the serial port for the virtual port %d. Wrote only %d bytes.", port, c);
-        return 0;
+        return -RT_EIO;
     }

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 125f48d and 9836841.

📒 Files selected for processing (1)

• src/cmux.c

🔇 Additional comments (4)

src/cmux.c (4)

80-80: LGTM!

The return type change from rt_size_t to rt_ssize_t is appropriate for supporting error codes.

---

652-658: Excellent fix: memset moved after allocation check.

Moving memset to line 658 (after the null check at line 653) correctly prevents a potential null pointer dereference if the allocation fails.

---

834-847: LGTM! Type changes are consistent.

The return type and local variable len type changes from rt_size_t to rt_ssize_t are consistent with the cmux_send_data return type change. However, ensure that the error handling in cmux_send_data is corrected to return negative error codes (see previous comment).

---

859-929: LGTM! Return type change is appropriate.

The return type change from rt_size_t to rt_ssize_t is forward-compatible and allows for future error handling improvements with negative error codes.