You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/06/03 08:42:03 UTC
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 opened a new pull request, #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
xiaoxiang781216 opened a new pull request, #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186
## Summary
follow nuttx kernel side change: https://github.com/apache/incubator-nuttx/pull/6357
## Impact
Minor
## Testing
Pass CI
--
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
[GitHub] [incubator-nuttx-apps] davids5 commented on pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#issuecomment-1145880364
This change hides the intent the the file is opened in binary mode. Does it make sense to assume the value the flag has?
--
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
[GitHub] [incubator-nuttx-apps] pkarashchenko merged pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186
--
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
[GitHub] [incubator-nuttx-apps] davids5 commented on pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#issuecomment-1146766720
@pkarashchenko Thank you for asking. I see it as a breaking changes, that does not add much value. But at least the compile will fail and trigger the hunt for "What! Why is NuttX not building again?"
--
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
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#issuecomment-1146756711
In general there is no harm in keeping the binary flag in code even if it is 0. But we can remove it of course.
--
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
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#issuecomment-1146206971
> This change hides the intent the the file is opened in binary mode. Does it make sense to assume the value the flag has?
Most caller of open want the binary mode behavior:
https://github.com/apache/incubator-nuttx-apps/search?q=open
but only these two places add O_BINARY. So the implementation of open need hold the following assumption:
File must be opened in binary mode if both O_BINARY and O_TEXT isn't passed to open.
Actually, other POSIX compliant OS doesn't support O_BINAY and O_TEXT at all:
https://stackoverflow.com/questions/2266992/no-o-binary-and-o-text-flags-in-linux
So, this change just make the code more consistent.
--
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
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#issuecomment-1146779724
> @xiaoxiang781216 Thank you for detailed explanation. It would be appreciated if your PR would include that sort of information when submitted.
Ok.
--
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
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#issuecomment-1146767600
> @pkarashchenko Thank you for asking. I see it as a breaking changes, that does not add much value. But at least the compile will fail and trigger the hunt for "What! Why is NuttX not building again?"
It isn't a breaking change: both flag still exist. I modify these two files just want to make sure the assumption more clear and consistent with the rest code in the repo.
--
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
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#issuecomment-1146756336
@davids5 are you ok with this PR and PR to the OS side?
--
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
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a diff in pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#discussion_r889675891
##########
netutils/ftpd/ftpd.c:
##########
@@ -1809,9 +1809,6 @@ static int ftpd_stream(FAR struct ftpd_session_s *session, int cmdtype)
#if defined(O_LARGEFILE)
oflags |= O_LARGEFILE;
#endif
-#if defined(O_BINARY)
Review Comment:
Sure
--
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
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on a diff in pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#discussion_r889704059
##########
netutils/ftpd/ftpd.c:
##########
@@ -1809,9 +1809,6 @@ static int ftpd_stream(FAR struct ftpd_session_s *session, int cmdtype)
#if defined(O_LARGEFILE)
oflags |= O_LARGEFILE;
#endif
-#if defined(O_BINARY)
Review Comment:
Posted comment on a wrong line, but still... I've checked can-utils and do not see any reference to O_BINARY
--
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
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on a diff in pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#discussion_r889670978
##########
netutils/ftpd/ftpd.c:
##########
@@ -1809,9 +1809,6 @@ static int ftpd_stream(FAR struct ftpd_session_s *session, int cmdtype)
#if defined(O_LARGEFILE)
oflags |= O_LARGEFILE;
#endif
-#if defined(O_BINARY)
Review Comment:
Let's check the original canutils code. If it contains this code, then it is better to keep it even if the binary flag is zero
--
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
[GitHub] [incubator-nuttx-apps] davids5 commented on pull request #1186: Remove "| O_BINARY" or " |= O_BINARY" since O_BINARY is 0 now
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #1186:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1186#issuecomment-1146771281
@xiaoxiang781216 Thank you for detailed explanation. It would be appreciated if your PR would include that sort of information when submitted.
--
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