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 2020/02/20 02:43:41 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #326: fcntl: add
O_CLOEXEC flag
anchao opened a new pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326
Change-Id: I08253dbf5e8b62ceffb511d64645e19a98653241
Signed-off-by: chao.an <an...@xiaomi.com>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] anchao commented on a change in pull request
#326: fcntl: add O_CLOEXEC/FD_CLOEXEC support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #326: fcntl: add O_CLOEXEC/FD_CLOEXEC support
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381803422
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
Change have been updated, Please help to review again, thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #326: fcntl: add O_CLOEXEC flag
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381708225
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
Because, this flag isn't very critical, some application just add this flag when it call open, but don't care the difference really.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #326:
fcntl: add O_CLOEXEC flag
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381700626
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
what's the point to make the app compile? it likely doesn't work as expected. a compile-time error is even better.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#326: fcntl: add O_CLOEXEC flag
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381690651
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
Are you going to implement support for this flag? Or is is just to complete a compile. If it is just for the compile why not set it zero.
That is what is done for the other flags below under "Unsupported, but required open flags"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#326: fcntl: add O_CLOEXEC flag
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381697798
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
The zero would be better value. This wastes a bit and we may want to reuse for another purpose in the future.
I should be moved to a section like;
/* Unsupported, but needed for Linux compatibility */
#define O_CLOEXEC (0) /* Close on execute */
That makes you intention clearer and does not waste a bit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] anchao commented on a change in pull request
#326: fcntl: add O_CLOEXEC flag
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381700395
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
Got, which seems more appropriate.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] anchao commented on a change in pull request
#326: fcntl: add O_CLOEXEC flag
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381695265
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
No implementation, just to ensure the compatibility of linux applications
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] anchao commented on a change in pull request
#326: fcntl: add O_CLOEXEC flag
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381709304
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
Yes, compile error is better but many applications come from other open source projects, in this case, flag O_CLOEXEC does not particularly affect the application behavior in my understanding, the change just make the application to run more smoothly in different OS.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] anchao commented on a change in pull request
#326: fcntl: add O_CLOEXEC flag
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381727834
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
Ok, I will upload the implementation later, please keep this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo merged pull request #326: fcntl: add
O_CLOEXEC/FD_CLOEXEC support
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #326: fcntl: add O_CLOEXEC/FD_CLOEXEC support
URL: https://github.com/apache/incubator-nuttx/pull/326
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #326:
fcntl: add O_CLOEXEC flag
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381713010
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
it might not be critical for some apps. it might be critical for others. we don't have a priori knowledge. an application for which this flag is not critical can always do #ifdef O_CLOEXEC. providing dummy definition can even break such applications.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#326: fcntl: add O_CLOEXEC flag
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #326: fcntl: add O_CLOEXEC flag
URL: https://github.com/apache/incubator-nuttx/pull/326#discussion_r381697798
##########
File path: include/fcntl.h
##########
@@ -66,6 +66,7 @@
#define O_DSYNC O_SYNC /* Equivalent to OSYNC in NuttX */
#define O_BINARY (1 << 8) /* Open the file in binary (untranslated) mode. */
#define O_DIRECT (1 << 9) /* Avoid caching, write directly to hardware */
+#define O_CLOEXEC (1 << 10) /* Close on execute */
Review comment:
The zero would be better value. This wastes a bit and we may want to reuse for another purpose in the future.
I should be moved to a section like;
/* Unsupported, but needed for Linux compatibility */
#define O_CLOEXEC (0) /* Close on execute */
That makes you intention clearer and does not waste a bit.
Most (but not all) unimplemented flags are set to zero. We should set all unimplemented flags to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services