ring_buffer: do not use the reserved byte

[giuseppe]

it is used for tracking of the status

Closes: https://issues.redhat.com/browse/OCPBUGS-59854
Closes: #1838

Summary by Sourcery

Prevent ring buffer operations from using the reserved byte, correct full-condition detection for wraparound, and enhance test coverage with comprehensive boundary and integrity scenarios

New Features:

• Add unit tests covering ring buffer wraparound data integrity, reserved-byte boundary conditions, and reserved-byte protection

Enhancements:

• Prevent ring buffer I/O vectors from reading or writing the reserved byte by adjusting iovec lengths
• Fix buffer-full detection to correctly use modulo arithmetic for wraparound
• Allow channel_fd_pair_process to trigger reads when any space is available instead of requiring full capacity

Tests:

• Update test runner to execute all four ring buffer tests

[sourcery-ai]

Reviewer's Guide

This PR enhances buffer safety by adding tests that verify reserved-byte handling and wraparound integrity, adjusts ring_buffer read/write IOV logic to exclude the reserved slot and fix the fullness check, and updates the channel processing logic to correctly detect available space.

Sequence diagram for channel_fd_pair_process buffer space check

sequenceDiagram
    participant Channel as channel_fd_pair
    participant RB as ring_buffer
    Channel->>RB: ring_buffer_get_space_available()
    alt space available > 0
        Channel->>RB: ring_buffer_read(...)
    else
        Channel-->>Channel: Skip read (buffer full)
    end

Loading

Class diagram for updated ring_buffer logic

classDiagram
    class ring_buffer {
        +char* buffer
        +size_t head
        +size_t tail
        +size_t size
        +ring_buffer_get_read_iov(struct iovec *iov)
        +ring_buffer_get_write_iov(struct iovec *iov)
        +ring_buffer_get_space_available()
        +ring_buffer_get_size()
    }
    class channel_fd_pair {
        +ring_buffer* rb
        +int in_fd
        +channel_fd_pair_process(int epollfd, libcrun_error_t *err)
    }
    ring_buffer <.. channel_fd_pair : uses

Loading

Flow diagram for reserved byte handling in ring_buffer

flowchart TD
    A[Start Read/Write Operation] --> B{Is operation at buffer end?}
    B -- Yes --> C[Exclude reserved byte from IOV length]
    B -- No --> D[Proceed with normal IOV calculation]
    C --> E[Return IOV]
    D --> E
    E --> F[Operation Complete]

Loading

File-Level Changes

Change	Details	Files
Expanded ring buffer test coverage	Added wraparound data integrity test Added reserved-byte boundary test Added reserved-byte access prevention test Updated test runner to include new tests	tests/tests_libcrun_ring_buffer.c
Prevent use of reserved byte in ring_buffer I/O vectors	Subtract 1 from read iov length to skip reserved byte Subtract 1 from write iov length to skip reserved byte Change full-buffer check to modulo-based comparison	src/libcrun/ring_buffer.c
Refine channel processing space check	Change buffer-space condition from >= size to > 0 for reads	src/libcrun/utils.c

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey @giuseppe - I've reviewed your changes - here's some feedback:

• test_ring_buffer_reserved_byte_boundary defines test_cases but never applies head/tail to the buffer or asserts should_be_full—update the test to actually set rb->head/rb->tail and verify full/not‐full behavior per case.
• ring_buffer_get_read_iov and ring_buffer_get_write_iov both use "-1" to skip the reserved byte; consider extracting this into a named constant or helper function to reduce duplication and improve clarity.
• Hardcoding the TAP plan header as "1..4" in main() is error-prone; consider computing the test count automatically or using a macro so it stays in sync when tests are added or removed.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- test_ring_buffer_reserved_byte_boundary defines test_cases but never applies head/tail to the buffer or asserts should_be_full—update the test to actually set rb->head/rb->tail and verify full/not‐full behavior per case.
- ring_buffer_get_read_iov and ring_buffer_get_write_iov both use "-1" to skip the reserved byte; consider extracting this into a named constant or helper function to reduce duplication and improve clarity.
- Hardcoding the TAP plan header as "1..4" in main() is error-prone; consider computing the test count automatically or using a macro so it stays in sync when tests are added or removed.

## Individual Comments

### Comment 1
<location> `src/libcrun/utils.c:2782` </location>
<code_context>
   for (i = 0, repeat = true; i < 1000 && repeat; i++)
     {
       repeat = false;
-      if (ring_buffer_get_space_available (channel->rb) >= ring_buffer_get_size (channel->rb))
+      if (ring_buffer_get_space_available (channel->rb) > 0)
         {
           ret = ring_buffer_read (channel->rb, channel->in_fd, &is_input_eagain, err);
</code_context>

<issue_to_address>
The new condition allows processing when there is any space, not just when the buffer is empty.

Please confirm that processing when any space is available is intentional, as this may impact throughput or blocking behavior.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

question (bug_risk): The new condition allows processing when there is any space, not just when the buffer is empty.

Please confirm that processing when any space is available is intentional, as this may impact throughput or blocking behavior.

[giuseppe]

@kolyshkin @flouthoc PTAL

[giuseppe]

@eriksjolund PTAL

[flouthoc]

LGTM