Re: [Qemu-devel] [for-2.10 PATCH] 9pfs: local: fix fchmodat_nofollow() limitations

[Eric Blake]

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...]

>> +++ 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
>> don't
>> +     * 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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature