5 对ringbuffer中rt_ringbuffer_put_force函数的疑问

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

最近研究驱动框架，顺带研读了ringbuffer源代码，发现一处疑问，个人认为是bug，也可能能力有限分析不对，请各位大佬批评指正，代码如下：

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长度的数据，从下面代码可以看出来：

    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数据，但是代码到下面这几行，存在问题了：

这里是一条华丽的分界线

    if (length > space_length)
    {
        rb->read_mirror = ~rb->read_mirror;
        rb->read_index = rb->write_index;
    }

这里是length >= buffer_size - write_index的情况，不然前面的代码直接return了。针对这种情况：

write_index一定会溢出（也就是会超过buffer_size），这个时候一定有write_mirror = ~write_mirror。（问题来了：read_index是否会溢出？这得分情况讨论，看下面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不变。

附一张截图，供大家理解我的意思：

所以我认为应该这样改:

     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 取反，则会导致环形缓冲区的数据长度清空，而不是满。

最后是关于楼主修改的部分是有问题的，应该这样改

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.

另外提供一份测试代码，方便测试：

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));
}

arminker 2021-07-16

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

@123 谢谢你的耐心解答，分析十分到位，所见略同，我虽然分析出了这个bug，但最后在代码改动上确实考虑不全面。
关于“溢出”这个词，我本意是write_index和read_index超过了缓冲区边界，为什么用这个词，也是看到原来代码作者的注释使用了overflow一词，故用之，谢谢你指出这个说法的不严谨之处。
最后总结下:
讨论length > space_length，是为了判断是否要修改read_index，至于read_mirror是否需要修改，只有在情况1时候才需要修改。
其实也就是你说的“充分考虑length > space_length时的情况”

请登录后再发布评论，点击登录

追加回复

5 个回答

aozima 2021-07-16

调网络不抓包，调I2C等时序不上逻辑分析仪，就像电工不用万用表！多用整理的好的文字，比截图更省流量，还能在整理过程中思考。

笔记好评！（草书看不懂，先夸再说 :-( ）

arminker 2021-07-16

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

不好意思，写得不好。不过也可以不看图的，图只是辅助理解

请登录后再发布评论，点击登录

追加回复

玩具箱 2021-07-16

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

不明白为什么会认为write_index一定会溢出？

arminker 2021-07-16

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

在buffer_size - write_index <= length时，write_index肯定会超过数组边界啊

请登录后再发布评论，点击登录

追加回复

flyboy 2021-07-23

Do my self();

厉害了，谁来把测试用例提交上来?

https://github.com/RT-Thread/rt-thread/tree/master/examples/utest

zhang515140166 2022-04-27

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

if (length > space_length)
    {
        rb->read_mirror = ~rb->write_mirror;
        rb->read_index = rb->write_index;
    }

这样改就可以了。

5 对ringbuffer中rt_ringbuffer_put_force函数的疑问

目录

情况2

情况1

撰写答案

关于作者

arminker

相关问题

热门标签

本月文章贡献

5 对ringbuffer中rt_ringbuffer_put_force函数的疑问

目录

情况2

情况1

撰写答案

关于作者

arminker

相关问题

推荐文章

最新文章

热门标签

本月问答贡献

本月文章贡献