examples/pipe: fix write usage #2829

Cynerd · 2024-11-07T13:22:16Z

Summary

write returns in case some bytes were written but not everything can fit.

This wasn't the case in NuttX but commit
d0680fd1bc51b7ead0b068fb24a31a144d22dc6c introduced this standard behavior.

Impact

This should fix CI as reported in apache/nuttx#14667 (comment).

Testing

pipe was executed on custom same7 board and passed.

> pipe

pipe_main: Performing FIFO test
open_write_only: Opening FIFO for write access
open_write_only: Waiting for open_write_only thread
pipe_main: open_write_only returned 0
transfer_test: fdin=4 fdout=3
transfer_test: Starting transfer_reader thread
transfer_test: Starting transfer_writer thread
transfer_reader: started
transfer_test: Waiting for transfer_writer thread
transfer_writer: started
transfer_writer: 18200 bytes written
transfer_reader: 18200 bytes read
transfer_test: transfer_writer returned 0
transfer_test: Waiting for transfer_reader thread
transfer_test: transfer_reader returned 0
transfer_test: returning 0
transfer_test: fdin=4 fdout=3
transfer_test: Boost priority of transfer_readerthread to 101
transfer_test: Starting transfer_reader thread
transfer_reader: started
transfer_test: Starting transfer_writer thread
transfer_test: Waiting for transfer_writer thread
transfer_writer: started
transfer_reader: 18200 bytes read
transfer_writer: 18200 bytes written
transfer_test: transfer_writer returned 0
transfer_test: Waiting for transfer_reader thread
transfer_test: transfer_reader returned 0
transfer_test: returning 0
transfer_test: fdin=4 fdout=3
transfer_test: Starting transfer_reader thread
transfer_test: Boost priority of transfer_writerthread to 101
transfer_reader: started
transfer_test: Starting transfer_writer thread
transfer_writer: started
transfer_test: Waiting for transfer_writer thread
transfer_writer: 18200 bytes written
transfer_reader: 18200 bytes read
transfer_test: transfer_writer returned 0
transfer_test: Waiting for transfer_reader thread
transfer_test: transfer_reader returned 0
transfer_test: returning 0
transfer_test: fdin=4 fdout=3
transfer_test: Boost priority of transfer_readerthread to 101
transfer_test: Starting transfer_reader thread
transfer_reader: started
transfer_test: Boost priority of transfer_writerthread to 101
transfer_test: Starting transfer_writer thread
transfer_writer: started
transfer_test: Waiting for transfer_writer thread
transfer_writer: 18200 bytes written
transfer_reader: 18200 bytes read
transfer_test: transfer_writer returned 0
transfer_test: Waiting for transfer_reader thread
transfer_test: transfer_reader returned 0
transfer_test: returning 0

pipe_main: Performing pipe interlock test
interlock_test: Starting null_writer thread
interlock_test: Opening FIFO for read access
null_writer: started -- sleeping
null_writer: Opening FIFO for write access
null_writer: Opened /var/testfifo-2 for writing -- sleeping
interlock_test: Reading from /var/testfifo-2
null_writer: Closing /var/testfifo-2
interlock_test: read returned
interlock_test: Closing /var/testfifo-2
interlock_test: Waiting for null_writer thread
null_writer: Returning success
interlock_test: writer returned 0
interlock_test: Starting null_reader thread
interlock_test: Opening FIFO for write access
null_reader: started -- sleeping
null_reader: Opening FIFO for read access
null_reader: Opened /var/testfifo-2 for reading -- sleeping
interlock_test: Writing to /var/testfifo-2
interlock_test: write returned
interlock_test: Closing /var/testfifo-2
interlock_test: Waiting for null_reader thread
null_reader: Closing /var/testfifo-2
null_reader: Returning success
interlock_test: reader returned 0
interlock_test: Returning 0
pipe_main: FIFO interlock test PASSED
pipe_main: FIFO test PASSED

pipe_main: Performing pipe test
transfer_test: fdin=4 fdout=3
transfer_test: Starting transfer_reader thread
transfer_test: Starting transfer_writer thread
transfer_reader: started
transfer_test: Waiting for transfer_writer thread
transfer_writer: started
transfer_writer: 18200 bytes written
transfer_reader: 18200 bytes read
transfer_test: transfer_writer returned 0
transfer_test: Waiting for transfer_reader thread
transfer_test: transfer_reader returned 0
transfer_test: returning 0
transfer_test: fdin=4 fdout=3
transfer_test: Boost priority of transfer_readerthread to 101
transfer_test: Starting transfer_reader thread
transfer_reader: started
transfer_test: Starting transfer_writer thread
transfer_test: Waiting for transfer_writer thread
transfer_writer: started
transfer_reader: 18200 bytes read
transfer_writer: 18200 bytes written
transfer_test: transfer_writer returned 0
transfer_test: Waiting for transfer_reader thread
transfer_test: transfer_reader returned 0
transfer_test: returning 0
transfer_test: fdin=4 fdout=3
transfer_test: Starting transfer_reader thread
transfer_test: Boost priority of transfer_writerthread to 101
transfer_reader: started
transfer_test: Starting transfer_writer thread
transfer_writer: started
transfer_test: Waiting for transfer_writer thread
transfer_writer: 18200 bytes written
transfer_reader: 18200 bytes read
transfer_test: transfer_writer returned 0
transfer_test: Waiting for transfer_reader thread
transfer_test: transfer_reader returned 0
transfer_test: returning 0
transfer_test: fdin=4 fdout=3
transfer_test: Boost priority of transfer_readerthread to 101
transfer_test: Starting transfer_reader thread
transfer_reader: started
transfer_test: Boost priority of transfer_writerthread to 101
transfer_test: Starting transfer_writer thread
transfer_writer: started
transfer_test: Waiting for transfer_writer thread
transfer_writer: 18200 bytes written
transfer_reader: 18200 bytes read
transfer_test: transfer_writer returned 0
transfer_test: Waiting for transfer_reader thread
transfer_test: transfer_reader returned 0
transfer_test: returning 0

pipe_main: Performing redirection test
redirection_test: Starting redirect_writer task with fd=3
redirection_test: Starting redirect_reader task with fd=4
redirect_writer: started with fdout=3
redirection_test: Waiting for null_reader thread
redirect_reader: started with fdin=4

Four score and seven years ago our fathers broughtforth on this continent a new nation,
conceived in Liberty, and dedicated to the propositionthat all men are created equal.

Now we are engaged in a great civil war, testingwhether that nation, or any nation, so
conceived and so dedicated, can long endure. We are meton a great battle-field of that war.
We have come to dedicate a portion of that field, as afinal resting place for those who hredirect_writer: 1456 bytes written
ere
gave their lives that that nationredirect_writer: Returning success
 might live. It isaltogether fitting and proper that we
should do this.

But, in a larger sense, we can not dedicate - we cannot consecrate - we can not hallow - this ground.
The brave men, living and dead, who struggled here, haveconsecrated it, far above our poor power
to add or detract. The world will little note, nor longremember what we say here, but it can
never forget what they did here. It is for us theliving, rather, to be dedicated here to the
unfinished work which they who fought here have thus farso nobly advanced. It is rather for us to
be here dedicated to the great task remaining before us- that from these honored dead we take
increased devotion to that cause for which they gave thelast full measure of devotion - that we
here highly resolve that these dead shall not have diedin vain - that this nation, under God,
shall have a new birth of freedom - and that governmentof the people, by the people, for the
people, shall not perish from the earth.

redirect_reader: 1456 bytes read
redirect_reader: Returning success
redirection_test: Waiting...
redirection_test: returning 0
pipe_main: PIPE redirection test PASSED
pipe_main: PIPE test PASSED

nuttxpr · 2024-11-07T13:23:43Z

[Experimental Bot, please feedback here]

No. This PR description lacks crucial information.

Here's what's missing and why it's important:

Insufficient Summary: "write returns..." is too vague. What specifically was changed in the code? Which function(s) were modified? What was the previous, incorrect behavior? The reference to the commit hash is helpful but insufficient; explain the change in human-readable terms. A link to the relevant NuttX Issue is missing.
Incomplete Impact Assessment: While it mentions fixing a CI issue, it doesn't fully address all the impact points. Consider:
- Impact on user: Does this change the behavior of applications using write? Even if the change aligns with POSIX, it's still a change that might affect user code.
- Impact on hardware: Unclear. While tested on a SAME7 board, does this change affect other architectures? Explicitly state "NO" or provide details if "YES." Similarly for drivers.
- Impact on documentation: Does this change require documentation updates?
- Impact on compatibility: Explicitly address backward compatibility. Does this change break existing NuttX applications?
- Impact on security: Consider if this change has any security ramifications. Even if the answer is "NO," it should be explicitly stated.
Inadequate Testing: "pipe was executed..." is far too vague. What were the specific test cases? What were the expected outputs before and after the change? The provided log sections are empty. Provide concrete examples of commands and their outputs, demonstrating both the previous broken behavior and the corrected behavior. Include details about the build host used for testing.

In short: The PR description needs to be significantly more detailed to meet the NuttX requirements. It must clearly articulate the change, its rationale, its impact across different aspects of the system, and provide verifiable testing evidence.

write returns in case some bytes were written but not everything can fit. This wasn't the case in NuttX but commit d0680fd1bc51b7ead0b068fb24a31a144d22dc6c introduced this standard behavior.

lupyuen · 2024-11-07T13:38:59Z

Thanks @Cynerd: I cloned your branch and enabled all the builds. I'll check the results here: https://github.com/nuttxpr/nuttx-apps/actions/runs/11724108240

Cynerd · 2024-11-07T13:53:37Z

Thanks @Cynerd: I cloned your branch and enabled all the builds. I'll check the results here: https://github.com/nuttxpr/nuttx-apps/actions/runs/11724108240

It still seems to fail for what ever reason. I will try those tests locally and see what is going on there.

Edit: Timeouts will be most likely caused by the pipe test because all tests afterward must timeout because pipe won't terminate, and thus, you can't spawn subsequent commands.

Cynerd · 2024-11-07T15:43:57Z

@lupyuen is there a way to get logs from CI for arm-02 board sabre-6quad. That seems to be the only one that failed. Other runs seem to be running fast again. The only one that failed is sabre-6quad. I am trying to run tests for it locally and it passes. I see that tests create logs in my case, but there is no such file in the arm-02 artifact, and thus, I am unable to find out where it actually failed.

lupyuen · 2024-11-07T15:48:27Z

@Cynerd Sorry do you mean this log? I ran sabre-6quad:citest 3 hours ago:
https://github.com/NuttX/nuttx/actions/runs/11722647465/job/32652723982#step:7:115

lupyuen · 2024-11-07T15:57:39Z

rv-virt:citest also failed 12 hours ago, according to nuttx-dashboard.org
https://gist.github.com/jerpelea/45f67d6dd6efba585b7a1e7a1f2c1216#file-ci-risc-v-05-log-L78

Is something inside the Docker Config that's causing it to fail?

Cynerd · 2024-11-07T15:59:14Z

I mean this file (of course different time in the name): nuttx/boards/arm/imx6/sabre-6quad/configs/citest/logs/sabre-6quad/qemu/sabre-6quad_20241107_162834.log

It is log from pexpect.

lupyuen · 2024-11-07T16:11:54Z

Sorry what's the command to run citest? I have compiled sabre-6quad:citest in Docker, but I'm not sure how to run it:

tools/configure.sh sabre-6quad:citest
make
## What next?

Cynerd · 2024-11-07T16:12:50Z

python3 -m pytest -m qemu tools/ci/testrun/script -B sabre-6quad -P . -L boards/arm/imx6/sabre-6quad/configs/citest/logs/sabre-6quad/qemu -R qemu -k 'pipe'

lupyuen · 2024-11-07T16:22:21Z

Is this correct? https://gist.github.com/nuttxpr/67b041a3a537343ebf08b46ed4b68b6b

ABDGHIJKNOPQ
CHelloWorld: Constructor: mSecret=42

NuttShell (NSH) NuttX-12.7.0
nsh> �[Kpipe

pipe_main: Performing FIFO test
open_write_only: Opening FIFO for write access
open_write_only: Waiting for open_write_only thread
pipe_main: open_write_only returned 0
transfer_test: fdin=4 fdout=3
transfer_test: Starting transfer_reader thread
transfer_test: Starting transfer_writer thread
transfer_test: Waiting for transfer_writer thread
transfer_reader: started
transfer_writer: started
transfer_writer: Unexpected write size=10
transfer_test: transfer_writer returned 2
transfer_test: Waiting for transfer_reader thread

Cynerd · 2024-11-07T16:48:47Z

Well it should have been https://gist.github.com/Cynerd/7285ef024ee7bf49518ee538ba9abc27. It seems that really this for some reason doesn't work in case of sabre-6quad. This just blocks it right after first test.

lupyuen · 2024-11-07T16:49:40Z

Or something special about the way QEMU runs inside Docker?

Cynerd · 2024-11-07T16:51:39Z

Or something special about the way QEMU runs inside Docker?

Possibly.. but why just for this one specific board? Or is it the only board that runs tests in Qemu? I am trying to run it in docker as well now. Will see..

Cynerd · 2024-11-07T16:55:03Z

Anyway I would be for merging this to fix at least CI runtime. The issue with sabre-6quad we could solve down the line.

lupyuen · 2024-11-07T16:56:05Z

BTW rv-virt:citest is also failing with QEMU inside Docker:
https://gist.github.com/jerpelea/45f67d6dd6efba585b7a1e7a1f2c1216#file-ci-risc-v-05-log-L78

Cynerd · 2024-11-07T17:00:53Z

@lupyuen can you please compare what you are doing with what I am doing? The full log of attempt in the docker: https://gist.github.com/Cynerd/81262ceaa32ac5719b453269e620cb0c

lupyuen · 2024-11-07T17:19:38Z

@Cynerd I ran the exact same steps and it works OK: https://gist.github.com/nuttxpr/bdb03952f774e711bafc4e7ae7d329c2

In the earlier log I used: git clone https://github.com/apache/nuttx-apps apps

That's the only difference I think.

github-actions bot added Area: Examples Size: S labels Nov 7, 2024

Cynerd mentioned this pull request Nov 7, 2024

drivers/pipes: return after short write if buffer is full apache/nuttx#14667

Merged

lupyuen approved these changes Nov 7, 2024

View reviewed changes

examples/pipe: fix write usage

8a806e8

write returns in case some bytes were written but not everything can fit. This wasn't the case in NuttX but commit d0680fd1bc51b7ead0b068fb24a31a144d22dc6c introduced this standard behavior.

Cynerd force-pushed the pipe-write branch from 953b2bd to 8a806e8 Compare November 7, 2024 13:27

xiaoxiang781216 approved these changes Nov 7, 2024

View reviewed changes

lupyuen mentioned this pull request Nov 7, 2024

add cmake compilation scripts for quickjs #2827

Open

xiaoxiang781216 approved these changes Nov 7, 2024

View reviewed changes

xiaoxiang781216 merged commit 87ce33b into apache:master Nov 7, 2024
25 checks passed

lupyuen mentioned this pull request Nov 7, 2024

tools/ci/platforms/darwin.sh: Port NuttX CI Job to macOS-14 apache/nuttx#14691

Merged

xiaoxiang781216 mentioned this pull request Nov 8, 2024

arch/stm32h5: Add ethernet hardware support files apache/nuttx#14692

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

examples/pipe: fix write usage #2829

examples/pipe: fix write usage #2829

Cynerd commented Nov 7, 2024 •

edited

Loading

nuttxpr commented Nov 7, 2024

lupyuen commented Nov 7, 2024

Cynerd commented Nov 7, 2024 •

edited

Loading

Cynerd commented Nov 7, 2024

lupyuen commented Nov 7, 2024

lupyuen commented Nov 7, 2024 •

edited

Loading

Cynerd commented Nov 7, 2024 •

edited

Loading

lupyuen commented Nov 7, 2024

Cynerd commented Nov 7, 2024

lupyuen commented Nov 7, 2024

Cynerd commented Nov 7, 2024

lupyuen commented Nov 7, 2024

Cynerd commented Nov 7, 2024

Cynerd commented Nov 7, 2024

lupyuen commented Nov 7, 2024

Cynerd commented Nov 7, 2024

lupyuen commented Nov 7, 2024 •

edited

Loading

examples/pipe: fix write usage #2829

examples/pipe: fix write usage #2829

Conversation

Cynerd commented Nov 7, 2024 • edited Loading

Summary

Impact

Testing

nuttxpr commented Nov 7, 2024

lupyuen commented Nov 7, 2024

Cynerd commented Nov 7, 2024 • edited Loading

Cynerd commented Nov 7, 2024

lupyuen commented Nov 7, 2024

lupyuen commented Nov 7, 2024 • edited Loading

Cynerd commented Nov 7, 2024 • edited Loading

lupyuen commented Nov 7, 2024

Cynerd commented Nov 7, 2024

lupyuen commented Nov 7, 2024

Cynerd commented Nov 7, 2024

lupyuen commented Nov 7, 2024

Cynerd commented Nov 7, 2024

Cynerd commented Nov 7, 2024

lupyuen commented Nov 7, 2024

Cynerd commented Nov 7, 2024

lupyuen commented Nov 7, 2024 • edited Loading

Cynerd commented Nov 7, 2024 •

edited

Loading

Cynerd commented Nov 7, 2024 •

edited

Loading

lupyuen commented Nov 7, 2024 •

edited

Loading

Cynerd commented Nov 7, 2024 •

edited

Loading

lupyuen commented Nov 7, 2024 •

edited

Loading