This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]