This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2 3/3] posix: Implement preadv2 and pwritev2
On 02/05/2017 15:01, Zack Weinberg wrote:
> On Tue, May 2, 2017 at 1:37 PM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>> On 02/05/2017 11:37, Christoph Hellwig wrote:
>>> On Tue, May 02, 2017 at 11:33:32AM -0300, Adhemerval Zanella wrote:
>>>> So default implementation just ignore the flag argument and calls generic
>>>> preadv/pwritev.
>>>
>>> Please don't do that, it's actively dangerous. Just fail the call.
>>
>> This a misleading commit comment from my part (I forgot to actually update
>> the commit log). In fact, I am following the suggestion from Zack and
>> returning EINVAL for unsupported flags in generic implementation (which
>> are currently none).
>
> This is true for the stub implementation (always returns -1/ENOSYS)
> and the sysdeps/posix implementation (returns -1/EINVAL if flags is
> nonzero, else calls preadv/pwritev, which is the Right Thing) ... but
> the sysdeps/unix/sysv/linux implementation appears to do the wrong
> thing when kernel support isn't available:
>
> +ssize_t
> +preadv2 (int fd, const struct iovec *vector, int count, off_t offset,
> + int flags)
> +{
> +# ifdef __NR_preadv2
> + ssize_t result = SYSCALL_CANCEL (preadv2, fd, vector, count,
> + LO_HI_LONG (offset), flags);
> + if (result >= 0 || errno != ENOSYS)
> + return result;
> +# endif
> + /* Trying to emulate the preadv2 syscall flags is troublesome:
> +
> + * We can not temporary change the file state of the O_DSYNC and O_SYNC
> + flags to emulate RWF_{D}SYNC (attempts to change the state of using
> + fcntl silently ignored).
> +
> + * IOCB_HIPRI requires the file opened in O_DIRECT and uses an internal
> + semantic not provided by any other flag (O_NONBLOCK for instance).
> +
> + So for generic case just ignore the flags can call preadv directly. */
> + return preadv (fd, vector, count, offset);
> +}
>
> The fallback logic needs to check whether flags is nonzero before
> calling preadv.
>
> (Relying on the kernel to reject unrecognized flags, when the syscall
> *is* available, seems reasonable to me.)
>
> zw
Indeed, it needs update as for default sysdeps/posix one. I will add, along
with a missing explanation about not using __ASSUME for this implementation
on kernel-features.h (as requested by Joseph).
>
> Note that the kernel returns -EOPNOTSUPP for not supported features,
> it would be good to have the same behavior in glibc.
Alright, I think it is reasonable returning EOPNOTSUPP for non supported
flags. I will change is as well and add on documentation.