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.

Sign up for GitHub

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

Jump to bottom

sys/tsrb: bug fix for tsrb_get_one() #11624

Merged
merged 1 commit into from Jun 5, 2019
Merged

sys/tsrb: bug fix for tsrb_get_one() #11624

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion sys/tsrb/tsrb.c
View file
Open in desktop
Expand Up @@ -32,7 +32,7 @@ static char _pop(tsrb_t *rb)
int tsrb_get_one(tsrb_t *rb)
{
if (!tsrb_empty(rb)) {
return _pop(rb);
return (unsigned char)_pop(rb);
Copy link
Member

Choose a reason for hiding this comment

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

To me the problem lies in mixing error return codes with values. A better solution would be to change the signature of the function into: int tsrb_get_one(tsrb_t *rb, char *dst). Also the same problem is otherwise with tsrb_get() below.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree with changing the signature of the API-facing tsrb_get_one() / tsrb_get(). Rather, _pop()'s return value should just be cast to unsigned char. Since sizeof(unsigned char) < sizeof(int) on all of our platforms, this is acceptable and does not mix error return codes with values.

Copy link
Member

Choose a reason for hiding this comment

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

this still looks wrong to me bc it uses char not unsigned char when adding stuff also the buffer is char. It just does not look really (type) safe to me - I would use uint8_t to make it explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO casting to unsigned char is the best option here. It is what getchar is doing, so can be called ideomatic C.
Also we might get away with calling this a bugfix, as the documentation clearly states that it returns the byte as >= 0. Now that I think of it, the cast must have been my initial intention anyways.

Copy link
Member

Choose a reason for hiding this comment

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

this still looks wrong to me bc it uses char not unsigned char when adding stuff also the buffer is char. It just does not look really (type) safe to me - I would use uint8_t to make it explicit.

In my opinion, we should focus on fixing the bug first, than we can discuss API changes for type safeness. The bug is fixable without an API change, so we should do that first, then do an API change if necessary. This not only makes the merge of this bug fix faster, but also allows for a better understanding of the thought process of the API change when looking at the code's history.

Copy link
Contributor

Choose a reason for hiding this comment

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

@smlng are you fine with this fix for now, and we revisit the API later?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, but it should not be that much later.

Another idea for a fix:

Documentation states, that this assumes a single producer and consumer scenario, right? So actually there is no need to return -1 on error but rather change the function to char tsrb_get_one() and the consumer MUST call tsrb_empty before access on its own otherwise the consumer might get an old/invalid value. This works because there is only one consumer hence there can be no other consumer manipulating the reads.

Still it might be sensible to change every char to unsigned char anyway.

}
else {
return -1;
Expand Down