bug反馈 ringbuffer

5 对ringbuffer中rt_ringbuffer_put_force函数的疑问

发布于 2021-07-16 10:06:17 浏览：1036 订阅该版

[tocm]

最近研究驱动框架，顺带研读了ringbuffer源代码，发现一处疑问，个人认为是bug，也可能能力有限分析不对，请各位大佬批评指正，代码如下：
```c
rt_size_t rt_ringbuffer_put_force(struct rt_ringbuffer *rb,
                            const rt_uint8_t     *ptr,
                            rt_uint16_t           length)
{
    rt_uint16_t space_length;

RT_ASSERT(rb != RT_NULL);

space_length = rt_ringbuffer_space_len(rb);

if (length > rb->buffer_size)
    {
        ptr = &ptr[length - rb->buffer_size];
        length = rb->buffer_size;
    }

if (rb->buffer_size - rb->write_index > length)
    {
        /* read_index - write_index = empty space */
        memcpy(&rb->buffer_ptr[rb->write_index], ptr, length);
        /* this should not cause overflow because there is enough space for
         * length of data in current mirror */
        rb->write_index += length;

if (length > space_length)
            rb->read_index = rb->write_index;

return length;
    }

memcpy(&rb->buffer_ptr[rb->write_index],
           &ptr[0],
           rb->buffer_size - rb->write_index);
    memcpy(&rb->buffer_ptr[0],
           &ptr[rb->buffer_size - rb->write_index],
           length - (rb->buffer_size - rb->write_index));

/* we are going into the other side of the mirror */
    rb->write_mirror = ~rb->write_mirror;
    rb->write_index = length - (rb->buffer_size - rb->write_index);

/******************分析这里，似乎是有bug？？？ ****************/
    if (length > space_length)
    {
        rb->read_mirror = ~rb->read_mirror;
        rb->read_index = rb->write_index;
    }

return length;
}
RTM_EXPORT(rt_ringbuffer_put_force);
```
`rt_ringbuffer_put_force`函数，向buffer中put数据的策略为：
1）如果数据长度`length > buffer_size,`就将数据截断，只留后面的`buffer_size`长度的数据，从下面代码可以看出来：
```c
    if (length > rb->buffer_size)
    {
        ptr = &ptr[length - rb->buffer_size];
        length = rb->buffer_size;
    }
```
2）space_length为空闲区长度，如果`length > space_length`，就将buffer中老的数据覆盖掉。

的确，`rt_ringbuffer_put_force`是按照上面两条策略在put数据，但是代码到下面这几行，存在问题了：

这里是一条华丽的分界线

---

```c
    if (length > space_length)
    {
        rb->read_mirror = ~rb->read_mirror;
        rb->read_index = rb->write_index;
    }
```
这里是`length >= buffer_size - write_index`的情况，不然前面的代码直接return了。针对这种情况：
1. `write_index`一定会溢出（也就是会超过`buffer_size`），这个时候一定有`write_mirror = ~write_mirror`。（问题来了：`read_index`是否会溢出？这得分情况讨论，看下面2）
2. read_index 呢（`length > space_length, read_index`就一定会超过buffer_size？？？，我认为不一定  ），下面讨论：

### 情况2
`write_index < read_index`，这种情况下，空闲区域在缓冲区中间，有数据的区域在缓冲区两头，这种情况下，read_index和write_index都会溢出，因此有：
` read_mirror = ~read_mirror`

### 情况1
`read_index < write_index`，这种情况，有数据区域在缓冲区中间，缓冲区两头是空闲的，这个时候，write_index一定会溢出，但是read_index却不会溢出（因为前面限定了length最大不会超过buffer_size），这种情况下，read_mirror不变。
 
附一张截图，供大家理解我的意思：
 ![image.png](https://oss-club.rt-thread.org/uploads/20210716/3ae2ab545300acecb9932b79ce94d7a4.png.webp)

所以我认为应该这样改:
```c
     if (length > space_length && write_index ≤ read_index)
    {
        rb->read_mirror = ~rb->read_mirror;
        rb->read_index = rb->write_index;
    }
```
    
大家认为呢？

123 认证专家 2021-07-16

这家伙很懒，什么也没写！

@RongLiu 先表一表楼主，可以的，很细心。👍

首先这个不叫溢出，这个叫回环，环形buffer的回环属于正常操作。（而溢出是致命的错误，溢出有标准的解释和处理手段，不要乱起名字）。

然后是这个 `情况1`,分析的很有道理，`write_index` 回环的时候，`read_index` 不一定会触发回环，这个时候如果强行将 `read_mirror` 取反，则会导致 环形缓冲区的数据长度清空，而不是满。

最后是关于楼主修改的部分是有问题的，应该这样改
```c
if (length > space_length)
{
    if(rb->write_index <= rb->read_index)  /* 情况1 */
        rb->read_mirror = ~rb->read_mirror;

rb->read_index = rb->write_index;      /* 情况2 */
}
```
应该充分考虑当`length > space_length`时候的情况，也就是情况1和情况2，显然楼主分析完之后，代码没有考虑情况2.

另外提供一份测试代码，方便测试：
```c
void test(void)
{
    rt_uint8_t ring_buffer[64];  /* ringbuffer的缓冲区*/
    rt_memset(ring_buffer, -1, 64);   /* 先格式化为0xff */

struct rt_ringbuffer test_rb;
    rt_ringbuffer_init(&test_rb, ring_buffer, 64);
    
    
    rt_uint8_t write_buffer[40], read_buffer[40]; /* 读buffer缓冲区和写buffer缓冲区 */
    rt_memset(write_buffer, 0, 40);
    rt_ringbuffer_put(&test_rb, write_buffer, 40);
    rt_ringbuffer_get(&test_rb, read_buffer, 5);
    
    rt_kprintf("rt_ringbuffer_data_len = [%d]\n", rt_ringbuffer_data_len(&test_rb));

rt_uint8_t write_second_buffer[34];
    rt_memset(write_second_buffer, 1, 40);
    rt_ringbuffer_put_force(&test_rb, write_second_buffer, 34);
    rt_kprintf("rt_ringbuffer_data_len = [%d]\n", rt_ringbuffer_data_len(&test_rb));
}
```