You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "xiaoxiang781216 (via GitHub)" <gi...@apache.org> on 2024/03/06 18:00:50 UTC
[PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
xiaoxiang781216 opened a new pull request, #11857:
URL: https://github.com/apache/nuttx/pull/11857
## Summary
it's simpler and safer than file_fcntl(F_SETFL)
## Impact
code refactor only
## Testing
internal test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1517076764
##########
drivers/pipes/pipe.c:
##########
@@ -305,9 +303,9 @@ int pipe2(int fd[2], int flags)
/* Clear O_NONBLOCK if it was set previously */
- if ((flags & O_NONBLOCK) == 0)
+ if (!nonblock)
{
- ret = fcntl(fd[1], F_SETFL, flags & (~O_NONBLOCK));
+ ret = ioctl(fd[1], FIONBIO, &nonblock);
Review Comment:
there are two places to check O_NONBLOCK in this function, do you want all place changing to `flags & O_NONBLOCK`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis merged PR #11857:
URL: https://github.com/apache/nuttx/pull/11857
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1517056066
##########
drivers/pipes/pipe.c:
##########
@@ -305,9 +303,9 @@ int pipe2(int fd[2], int flags)
/* Clear O_NONBLOCK if it was set previously */
- if ((flags & O_NONBLOCK) == 0)
+ if (!nonblock)
{
- ret = fcntl(fd[1], F_SETFL, flags & (~O_NONBLOCK));
+ ret = ioctl(fd[1], FIONBIO, &nonblock);
Review Comment:
Why not reuse `ret` as in `net/local/local_fifo.c`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1517099632
##########
drivers/pipes/pipe.c:
##########
@@ -305,9 +303,9 @@ int pipe2(int fd[2], int flags)
/* Clear O_NONBLOCK if it was set previously */
- if ((flags & O_NONBLOCK) == 0)
+ if (!nonblock)
{
- ret = fcntl(fd[1], F_SETFL, flags & (~O_NONBLOCK));
+ ret = ioctl(fd[1], FIONBIO, &nonblock);
Review Comment:
The difference between two is minor, it's fine with either to me:).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#issuecomment-1982279978
> @xiaoxiang781216 did you try it: https://acassis.wordpress.com/2021/03/04/using-fifo-on-nuttx-to-send-data-from-your-board-to-computer/ ?
not try, but we use pipe in many places, so the test coverage is good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1517070590
##########
drivers/pipes/pipe.c:
##########
@@ -305,9 +303,9 @@ int pipe2(int fd[2], int flags)
/* Clear O_NONBLOCK if it was set previously */
- if ((flags & O_NONBLOCK) == 0)
+ if (!nonblock)
{
- ret = fcntl(fd[1], F_SETFL, flags & (~O_NONBLOCK));
+ ret = ioctl(fd[1], FIONBIO, &nonblock);
Review Comment:
I mean that in `net/local/local_fifo.c` you have something that can be used here
```
if ((flags & O_NONBLOCK) == 0)
{
ret = 1;
ret = ioctl(fd[1], FIONBIO, &ret);
}
```
and get rid of `nonblock`, so my ask was basically why different places implement different approaches?
##########
net/local/local_fifo.c:
##########
@@ -294,7 +294,8 @@ static int local_tx_open(FAR struct local_conn_s *conn, FAR const char *path,
if (nonblock == false)
Review Comment:
Note: `if (!nonblock)`
##########
net/local/local_fifo.c:
##########
@@ -294,7 +294,8 @@ static int local_tx_open(FAR struct local_conn_s *conn, FAR const char *path,
if (nonblock == false)
{
- ret = file_fcntl(&conn->lc_outfile, F_SETFL, O_WRONLY);
+ ret = nonblock;
Review Comment:
`ret = nonblock;` -> `ret = 1;` to be more explicit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1515374787
##########
drivers/pipes/pipe.c:
##########
@@ -207,8 +208,8 @@ static int pipe_register(size_t bufsize, int flags,
int file_pipe(FAR struct file *filep[2], size_t bufsize, int flags)
{
char devname[32];
+ int nonblock = !!(flags & O_NONBLOCK);
Review Comment:
Because file_vioctl expect int:
https://github.com/apache/nuttx/blob/master/fs/vfs/fs_ioctl.c#L75
which is mentioned here:
https://man.openbsd.org/ioctl.2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1517082257
##########
drivers/pipes/pipe.c:
##########
@@ -305,9 +303,9 @@ int pipe2(int fd[2], int flags)
/* Clear O_NONBLOCK if it was set previously */
- if ((flags & O_NONBLOCK) == 0)
+ if (!nonblock)
{
- ret = fcntl(fd[1], F_SETFL, flags & (~O_NONBLOCK));
+ ret = ioctl(fd[1], FIONBIO, &nonblock);
Review Comment:
yes, but in `file_pipe` and `pipe2` we can get rid of `nonblock` auto variable and just reuse `ret = 1;`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1517068036
##########
drivers/pipes/pipe.c:
##########
@@ -305,9 +303,9 @@ int pipe2(int fd[2], int flags)
/* Clear O_NONBLOCK if it was set previously */
- if ((flags & O_NONBLOCK) == 0)
+ if (!nonblock)
{
- ret = fcntl(fd[1], F_SETFL, flags & (~O_NONBLOCK));
+ ret = ioctl(fd[1], FIONBIO, &nonblock);
Review Comment:
nonblock has the right type(int) here, but wrong type(bool) in local_tx_open, so the temp assign to ret doesn't need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1517076764
##########
drivers/pipes/pipe.c:
##########
@@ -305,9 +303,9 @@ int pipe2(int fd[2], int flags)
/* Clear O_NONBLOCK if it was set previously */
- if ((flags & O_NONBLOCK) == 0)
+ if (!nonblock)
{
- ret = fcntl(fd[1], F_SETFL, flags & (~O_NONBLOCK));
+ ret = ioctl(fd[1], FIONBIO, &nonblock);
Review Comment:
there are many place to check O_NONBLOCK in this function, do you want all place changing to `flags & O_NONBLOCK`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1517076764
##########
drivers/pipes/pipe.c:
##########
@@ -305,9 +303,9 @@ int pipe2(int fd[2], int flags)
/* Clear O_NONBLOCK if it was set previously */
- if ((flags & O_NONBLOCK) == 0)
+ if (!nonblock)
{
- ret = fcntl(fd[1], F_SETFL, flags & (~O_NONBLOCK));
+ ret = ioctl(fd[1], FIONBIO, &nonblock);
Review Comment:
there are two places to check O_NONBLOCK in this function, do you want all place changing to `flags & O_NONBLOCK`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#issuecomment-1981488114
@xiaoxiang781216 did you try it: https://acassis.wordpress.com/2021/03/04/using-fifo-on-nuttx-to-send-data-from-your-board-to-computer/ ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1515229273
##########
drivers/pipes/pipe.c:
##########
@@ -207,8 +208,8 @@ static int pipe_register(size_t bufsize, int flags,
int file_pipe(FAR struct file *filep[2], size_t bufsize, int flags)
{
char devname[32];
+ int nonblock = !!(flags & O_NONBLOCK);
Review Comment:
Why not to use bool?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/pipe: Change to the block mode by file_ioctl(FIONBIO) [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11857:
URL: https://github.com/apache/nuttx/pull/11857#discussion_r1517079507
##########
drivers/pipes/pipe.c:
##########
@@ -305,9 +303,9 @@ int pipe2(int fd[2], int flags)
/* Clear O_NONBLOCK if it was set previously */
- if ((flags & O_NONBLOCK) == 0)
+ if (!nonblock)
{
- ret = fcntl(fd[1], F_SETFL, flags & (~O_NONBLOCK));
+ ret = ioctl(fd[1], FIONBIO, &nonblock);
Review Comment:
both places use `nonblock`, the only difference is that `nonblock` has different type:
1. nonblock in pipe.c is local variable, so it's fine to change it's type to int to match FIONBIO's expectation
2. nonblock in local_fifo.c is function argument, so it is better to keep bool type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org