On 08/08/2017 04:34 PM, Eric Blake wrote:
On 08/08/2017 01:48 PM, Philippe Mathieu-Daudé wrote:

+    fd = openat_file(dirfd, name, O_RDONLY | O_PATH, 0);

since you use O_PATH, you can drop O_RDONLY.

Technically, POSIX says (and 'man 2 open' agrees, modulo the fact that
Linux still lacks O_SEARCH) that you MUST provide one of the 5 access
modes (they are O_RDONLY, O_RDWR, O_WRONLY, O_EXEC, and O_SEARCH;
although POSIX allows O_EXEC and O_SEARCH to be the same bit pattern),
and then bitwise-or any additional flags.  So the usage here is correct.

Practically, though, this code is ONLY compilable on Linux (no one else
has O_PATH yet, although it is a useful Linux invention), and on Linux,
O_RDONLY is 0.  So behaviorally, you are correct that open(O_PATH)
rather than open(O_RDONLY | O_PATH) does the same thing, even if it sets
a bad example. [Well, insofar as you don't have any other bugs by
omitting O_NOFOLLOW, per my other thread...]

Oh ok. I didn't think of that, just checked Linux manpage:

   O_PATH (since Linux 2.6.39)

          When O_PATH is specified in flags, flag bits other than
          O_CLOEXEC, O_DIRECTORY, and O_NOFOLLOW are ignored.

+++ b/hw/9pfs/9p-util.h
@@ -43,9 +43,13 @@ static inline int openat_file(int dirfd, const char
*name, int flags,
         serrno = errno;
-    /* O_NONBLOCK was only needed to open the file. Let's drop it. */
-    ret = fcntl(fd, F_SETFL, flags);
-    assert(!ret);
+    /* O_NONBLOCK was only needed to open the file. Let's drop it. We
+     * do that with O_PATH since it ignores O_NONBLOCK.
+     */

thinking about if someone uses openat_file(d, O_PATH|O_CLOEXEC, ...),
why not set the fd flags always? like:

        ret = fcntl(fd, F_SETFL, flags);
        assert((flags & O_PATH) || !ret);

Syscalls are expensive.  It's better to avoid fcntl() up front than it
is to skip failure after the fact.

Ok, good to know!

Thank to clarify those points,


Reply via email to