Skip to content

tests/pipe: cleanup application#12792

Merged
maribu merged 1 commit intoRIOT-OS:masterfrom
aabadie:pr/tests/pipe_cleanup
Nov 23, 2019
Merged

tests/pipe: cleanup application#12792
maribu merged 1 commit intoRIOT-OS:masterfrom
aabadie:pr/tests/pipe_cleanup

Conversation

@aabadie
Copy link
Copy Markdown
Contributor

@aabadie aabadie commented Nov 23, 2019

Contribution description

This PR is cleaning up the tests/pipe application to make it also work with AVR. I have to admit that I didn't know the printf syntax (%.*s) to print a string given a length and a buffer.
Apparently this doesn't work with the AVR toolchain so the fix is just to close the string with a null byte explicitly and use %s formatter in the print.

Testing procedure

  • A green Murdock with run tests enabled
  • The following should also work now:
$ make BOARD=atmega256rfr2-xpro -C tests/pipe flash test

Issues/PRs references

Tick one item in #12651

@aabadie aabadie added Area: tests Area: tests and testing framework Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Nov 23, 2019
@aabadie aabadie requested a review from maribu November 23, 2019 07:15
@aabadie aabadie added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Nov 23, 2019
@aabadie aabadie mentioned this pull request Nov 23, 2019
Closed
13 tasks
maribu
maribu reviewed Nov 23, 2019
View reviewed changes
tests/pipe/main.c
while (read_total < BYTES_TOTAL) {
char buf[4];
unsigned read = pipe_read(&pipes[0], buf, sizeof (buf));
buf[read] = 0;
Copy link
Copy Markdown
Member

@maribu maribu Nov 23, 2019

Choose a reason for hiding this comment

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

Potential buffer overflow by one byte. (pipe_read() reads up to sizeof(buf) bytes. I suggest to increase the buffer by one byte and read only sizeof(buf) - 1 bytes. Same below.

Copy link
Copy Markdown
Contributor Author

@aabadie aabadie Nov 23, 2019

Choose a reason for hiding this comment

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

Makes sense, thanks!

Copy link
Copy Markdown
Contributor Author

@aabadie aabadie Nov 23, 2019

Choose a reason for hiding this comment

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

And done!

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Nov 23, 2019

As a side note, the problem in tests/pipe with AVR has the same origin as the one in tests/pkg_jsmn. I could also fix it using the same strategy applied here in tests/pkg_jsmn but I'm wondering if it's the right "fix". Maybe some lower level, more AVR central code, could be adapted but I couldn't find where. My concern is that %.*s is a valid formatter in printf so I don't see any reason why that wouldn't work on AVR.
Do you have any idea @maribu ?

@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 23, 2019

It could be added to avr-libc. But that would be pretty long term till it reaches users. To me it actually makes sense that only a subset of printf formatters are available on low end systems.

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Nov 23, 2019

It could be added to avr-libc. But that would be pretty long term till it reaches users. To me it actually makes sense that only a subset of printf formatters are available on low end systems.

Thanks for your prompt reply @maribu. Then let's go with the strategy of this PR and I'll also open another PR for tests/pkg_jsmn.

@aabadie
tests/pipe: cleanup application
a081fb6 
The string formatter initially used doesn't seem to be supported by the AVR toolchain. Correctly closing the buffer with a null byte and using plain %s formatter works in all cases
@aabadie aabadie force-pushed the pr/tests/pipe_cleanup branch from f1df0e3 to a081fb6 Compare November 23, 2019 14:07
@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 23, 2019
@aabadie aabadie mentioned this pull request Nov 23, 2019
Merged
maribu
maribu approved these changes Nov 23, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Thanks for tackling this

@maribu maribu merged commit 54ee5eb into RIOT-OS:master Nov 23, 2019
@aabadie aabadie deleted the pr/tests/pipe_cleanup branch November 23, 2019 16:58
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maribu maribu maribu approved these changes

Assignees

No one assigned

Labels

Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.01

Development

Successfully merging this pull request may close these issues.

3 participants

@aabadie @maribu @fjmolinas