Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Libc ringbuff implementation #23

Merged
merged 15 commits into from
Dec 29, 2020
Merged

Libc ringbuff implementation #23

merged 15 commits into from
Dec 29, 2020

Conversation

xadaemon
Copy link
Contributor

I'm opening this PR with the libc ring-buffer mentioned in #15, I intend to submit a later pr fixing #15 itself but for now to allow usage elsewhere I decided to open this PR now.

I did some heavy commenting on the code in the hopes that it can sort of explain itself.

Copy link
Owner

@29jm 29jm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @octetd ! I had assumed you'd reuse the implementation from pipe.c, but I can dig wanting to reinvent that wheel :) Commented with a few issues I noted & other details.

libc/include/ringbuffer.h Outdated Show resolved Hide resolved
libc/include/ringbuffer.h Outdated Show resolved Hide resolved
libc/include/ringbuffer.h Outdated Show resolved Hide resolved
libc/include/ringbuffer.h Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
libc/include/ringbuffer.h Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
Copy link
Owner

@29jm 29jm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments. Those are the last stylistic changes I'll ask of you, promise ^^

For the _write changes, I'm trying to make sure we have a rock solid data structure here. Avoiding new memory corruption issues is a big deal to me, as those are always very time consuming later on when they inevitably cause issues in unrelated parts of the project. As it is, it's hard to reason about, but I'm sure there's an issue with at least the updating of unread_data.

libc/include/ringbuffer.h Show resolved Hide resolved
libc/include/ringbuffer.h Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
libc/src/ringbuffer.c Outdated Show resolved Hide resolved
@29jm
Copy link
Owner

29jm commented Dec 23, 2020

Just one last thing: unread_data is unbounded. Writing 100 bytes in a 10 bytes ringbuffer will tell you that there's -90 bytes of data available. Everything else looks great !

@xadaemon
Copy link
Contributor Author

Just one last thing: unread_data is unbounded. Writing 100 bytes in a 10 bytes ringbuffer will tell you that there's -90 bytes of data available. Everything else looks great !

should be bounded now

@29jm
Copy link
Owner

29jm commented Dec 24, 2020

Would you mind cherry-picking the last commit from the pr-ringbuffer branch, or making a commit with similar changes? Ring buffers are hard to get right, I tested thoroughly and found a few more issues:

  • r_pos wasn't being updated when data was overwritten; it should always point to the oldest data
  • unread_data was indeed bounded, but could now become smaller on writes ^^

I did away with w_pos, updating it independently from r_pos made things harder, since it's in fact entirely dependent on r_pos and unread_data.

Merging right after that!

@xadaemon
Copy link
Contributor Author

xadaemon commented Dec 24, 2020 via email

@xadaemon
Copy link
Contributor Author

small update, had a few personal issues, will get it cherry picked tomorrow

@xadaemon xadaemon requested a review from 29jm December 29, 2020 00:11
@xadaemon
Copy link
Contributor Author

@29jm I did the cherry-pick as requested ^^

Copy link
Owner

@29jm 29jm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfection 👌

@29jm 29jm merged commit 5bd487a into 29jm:master Dec 29, 2020
@29jm
Copy link
Owner

29jm commented Dec 29, 2020

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants