diff mbox

posix: Add p{read,write}v2 RWF_NOWAIT flag (BZ#21738)

Message ID 1499714685-12770-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella July 10, 2017, 7:24 p.m. UTC 
Linux 4.12 (b745fafaf70c0a98a2e1e7ac8cb14542889ceb0e) adds a new
p{read,write}v2 flag RWF_NOWAIT.  This patch adds it for linux
uio-ext.h header along with RWF_SUPPORTED (a mask with all supported
flags).

Checked on x86_64-linux-gnu (on a 4.10 kernel).

	[BZ #21738]
	* manual/llio.texi (RWF_NOWAIT): New item.
	(RWF_SUPPORTED): Likewise.
	* misc/tst-preadvwritev2-common.c (do_test_with_invalid_flags):
	Create an invalid flag from RWF_SUPPORTED definition.
	* sysdeps/unix/sysv/linux/bits/uio-ext.h (RWF_NOWAIT): New flag.
	(RWF_SUPPORTED): Likewise.
---
 ChangeLog                              | 10 ++++++++++
 manual/llio.texi                       |  6 ++++++
 misc/tst-preadvwritev2-common.c        | 14 ++++----------
 sysdeps/unix/sysv/linux/bits/uio-ext.h |  4 ++++
 4 files changed, 24 insertions(+), 10 deletions(-)

-- 
2.7.4

Comments

Florian Weimer July 10, 2017, 7:31 p.m. UTC | #1 
* Adhemerval Zanella:

> +@item RWF_SUPPORTED

> +Mask with all supported flags.


I assume the kernel intends to add further flags in the future.  I'm
not sure it's wise to provide this to applications because it's not
very likely that it's going to match the flag mask used by the kernel.
Zack Weinberg July 10, 2017, 7:34 p.m. UTC | #2 
On Mon, Jul 10, 2017 at 3:24 PM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> Linux 4.12 (b745fafaf70c0a98a2e1e7ac8cb14542889ceb0e) adds a new

> p{read,write}v2 flag RWF_NOWAIT.  This patch adds it for linux

> uio-ext.h header along with RWF_SUPPORTED (a mask with all supported

> flags).


LGTM with a couple of notes on the documentation:

> +@item RWF_NOWAIT

> +Set @code{preadv2} to return -EAGAIN if operation would block.


This is awkward grammar.  Also, this function sets errno, doesn't it?
"Return -EAGAIN" is an intra-kernel convention, not what user space
sees, which is what the manual should document.

Suggest instead "Use nonblocking mode for this operation; that is,
this call to @code{preadv2} will fail and set @code{errno} to
@code{EAGAIN} if the operation would block."

> +@item RWF_SUPPORTED

> +Mask with all supported flags.


It's important to be clear that this does not necessarily reflect the
running kernel:  "Bitmask of all @samp{RWF_*} flags that have so far
been defined.  This is a compile-time constant; the running kernel
does not necessarily support all of the flags defined in this header.
New flags were most recently added in Linux 4.12."

> +#define RWF_NOWAIT     0x00000008 /* per-IO, return -EAGAIN if operation

> +                                     would block */


Same as above: "per-IO nonblocking mode" would be sufficient here, I think.

zw
Zack Weinberg July 10, 2017, 7:36 p.m. UTC | #3 
Argh, one more thing:

On Mon, Jul 10, 2017 at 3:34 PM, Zack Weinberg <zackw@panix.com> wrote:
>

> Suggest instead "Use nonblocking mode for this operation; that is,

> this call to @code{preadv2} will fail and set @code{errno} to

> @code{EAGAIN} if the operation would block."


"... this call to @code{preadv2} or @code{pwritev2} ..." It does work
in both, right?

zw
Adhemerval Zanella July 10, 2017, 7:40 p.m. UTC | #4 
On 10/07/2017 16:34, Zack Weinberg wrote:
> On Mon, Jul 10, 2017 at 3:24 PM, Adhemerval Zanella

> <adhemerval.zanella@linaro.org> wrote:

>> Linux 4.12 (b745fafaf70c0a98a2e1e7ac8cb14542889ceb0e) adds a new

>> p{read,write}v2 flag RWF_NOWAIT.  This patch adds it for linux

>> uio-ext.h header along with RWF_SUPPORTED (a mask with all supported

>> flags).

> 

> LGTM with a couple of notes on the documentation:

> 

>> +@item RWF_NOWAIT

>> +Set @code{preadv2} to return -EAGAIN if operation would block.

> 

> This is awkward grammar.  Also, this function sets errno, doesn't it?

> "Return -EAGAIN" is an intra-kernel convention, not what user space

> sees, which is what the manual should document.

> 

> Suggest instead "Use nonblocking mode for this operation; that is,

> this call to @code{preadv2} will fail and set @code{errno} to

> @code{EAGAIN} if the operation would block."


> On Mon, Jul 10, 2017 at 3:34 PM, Zack Weinberg <zackw@panix.com> wrote:

>>

>> Suggest instead "Use nonblocking mode for this operation; that is,

>> this call to @code{preadv2} will fail and set @code{errno} to

>> @code{EAGAIN} if the operation would block."

> 

> "... this call to @code{preadv2} or @code{pwritev2} ..." It does work

> in both, right?

> 

> zw


Indeed the wording is unpolished, I will use your version instead,
although the topic is referenced to preadv2 (on pwritev2 topic there is
a note it accepts the same flags from preadv2).

> 

>> +@item RWF_SUPPORTED

>> +Mask with all supported flags.

> 

> It's important to be clear that this does not necessarily reflect the

> running kernel:  "Bitmask of all @samp{RWF_*} flags that have so far

> been defined.  This is a compile-time constant; the running kernel

> does not necessarily support all of the flags defined in this header.

> New flags were most recently added in Linux 4.12."


From Florian's comment I also think it does not worth the trouble to
provide it when it might be have different values than running kernel.
I will remove it.

> 

>> +#define RWF_NOWAIT     0x00000008 /* per-IO, return -EAGAIN if operation

>> +                                     would block */

> 

> Same as above: "per-IO nonblocking mode" would be sufficient here, I think.


Right (although I just copied and pasted the kernel comment).
Adhemerval Zanella July 10, 2017, 7:47 p.m. UTC | #5 
On 10/07/2017 16:36, Zack Weinberg wrote:
> Argh, one more thing:

> 

> On Mon, Jul 10, 2017 at 3:34 PM, Zack Weinberg <zackw@panix.com> wrote:

>>

>> Suggest instead "Use nonblocking mode for this operation; that is,

>> this call to @code{preadv2} will fail and set @code{errno} to

>> @code{EAGAIN} if the operation would block."

> 

> "... this call to @code{preadv2} or @code{pwritev2} ..." It does work

> in both, right?

> 

> zw

> 


That's the new version:

	[BZ #21738]
	* manual/llio.texi (RWF_NOWAIT): New item.
	* misc/tst-preadvwritev2-common.c (do_test_with_invalid_flags):
	Add RWF_NOWAIT check.
	* sysdeps/unix/sysv/linux/bits/uio-ext.h (RWF_NOWAIT): New flag.

---

-- 
2.7.4diff --git a/manual/llio.texi b/manual/llio.texi
index ba1f455..e72c53c 100644
--- a/manual/llio.texi
+++ b/manual/llio.texi
@@ -770,6 +770,10 @@ Per-IO synchronization as if the file was opened with @code{O_DSYNC} flag.
 
 @item RWF_SYNC
 Per-IO synchronization as if the file was opened with @code{O_SYNC} flag.
+
+@item RWF_NOWAIT
+Use nonblocking mode for this operation; that is, this call to @code{preadv2}
+will fail and set @code{errno} to @code{EAGAIN} if the operation would block.
 @end vtable
 
 When the source file is compiled with @code{_FILE_OFFSET_BITS == 64} the
diff --git a/misc/tst-preadvwritev2-common.c b/misc/tst-preadvwritev2-common.c
index 4c53d56..8abedc1 100644
--- a/misc/tst-preadvwritev2-common.c
+++ b/misc/tst-preadvwritev2-common.c
@@ -16,21 +16,16 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <limits.h>
 #include <support/check.h>
 
 static void
 do_test_with_invalid_flags (void)
 {
-  int invalid_flag = 0x1;
-#ifdef RWF_HIPRI
-  invalid_flag <<= 1;
-#endif
-#ifdef RWF_DSYNC
-  invalid_flag <<= 1;
-#endif
-#ifdef RWF_SYNC
-  invalid_flag <<= 1;
-#endif
+#define RWF_SUPPORTED	(RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT)
+  /* Set the next bit from the mask of all supported flags.  */
+  int invalid_flag = __builtin_clz (RWF_SUPPORTED);
+  invalid_flag = 0x1 << ((sizeof (int) * CHAR_BIT) - invalid_flag);
 
   char buf[32];
   const struct iovec vec = { .iov_base = buf, .iov_len = sizeof (buf) };
diff --git a/sysdeps/unix/sysv/linux/bits/uio-ext.h b/sysdeps/unix/sysv/linux/bits/uio-ext.h
index 4ddb644..12cd78e 100644
--- a/sysdeps/unix/sysv/linux/bits/uio-ext.h
+++ b/sysdeps/unix/sysv/linux/bits/uio-ext.h
@@ -46,6 +46,10 @@ extern ssize_t process_vm_writev (pid_t __pid, const struct iovec *__lvec,
 #define RWF_HIPRI	0x00000001 /* High priority request.  */
 #define RWF_DSYNC	0x00000002 /* per-IO O_DSYNC.  */
 #define RWF_SYNC	0x00000004 /* per-IO O_SYNC.  */
+#define RWF_NOWAIT	0x00000008 /* per-IO, return -EAGAIN if operation
+				      would block */
+#define RWF_SUPPORTED	(RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT)
+
 
 __END_DECLS
Markus Trippelsdorf July 11, 2017, 11:06 a.m. UTC | #6 
On 2017.07.10 at 16:47 -0300, Adhemerval Zanella wrote:
> 

> 

> On 10/07/2017 16:36, Zack Weinberg wrote:

> > Argh, one more thing:

> > 

> > On Mon, Jul 10, 2017 at 3:34 PM, Zack Weinberg <zackw@panix.com> wrote:

> >>

> >> Suggest instead "Use nonblocking mode for this operation; that is,

> >> this call to @code{preadv2} will fail and set @code{errno} to

> >> @code{EAGAIN} if the operation would block."

> > 

> > "... this call to @code{preadv2} or @code{pwritev2} ..." It does work

> > in both, right?

> > 

> > zw

> > 

> 

> That's the new version:

> 

> 	* sysdeps/unix/sysv/linux/bits/uio-ext.h (RWF_NOWAIT): New flag.


> diff --git a/sysdeps/unix/sysv/linux/bits/uio-ext.h b/sysdeps/unix/sysv/linux/bits/uio-ext.h

> index 4ddb644..12cd78e 100644

> --- a/sysdeps/unix/sysv/linux/bits/uio-ext.h

> +++ b/sysdeps/unix/sysv/linux/bits/uio-ext.h

> @@ -46,6 +46,10 @@ extern ssize_t process_vm_writev (pid_t __pid, const struct iovec *__lvec,

>  #define RWF_HIPRI	0x00000001 /* High priority request.  */

>  #define RWF_DSYNC	0x00000002 /* per-IO O_DSYNC.  */

>  #define RWF_SYNC	0x00000004 /* per-IO O_SYNC.  */

> +#define RWF_NOWAIT	0x00000008 /* per-IO, return -EAGAIN if operation

> +				      would block */

> +#define RWF_SUPPORTED	(RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT)


In case you haven't noticed already, this still defines RWF_SUPPORTED in
uio-ext.h.

-- 
Markus
Adhemerval Zanella July 11, 2017, 1:02 p.m. UTC | #7 
On 11/07/2017 08:06, Markus Trippelsdorf wrote:
> On 2017.07.10 at 16:47 -0300, Adhemerval Zanella wrote:

>>

>>

>> On 10/07/2017 16:36, Zack Weinberg wrote:

>>> Argh, one more thing:

>>>

>>> On Mon, Jul 10, 2017 at 3:34 PM, Zack Weinberg <zackw@panix.com> wrote:

>>>>

>>>> Suggest instead "Use nonblocking mode for this operation; that is,

>>>> this call to @code{preadv2} will fail and set @code{errno} to

>>>> @code{EAGAIN} if the operation would block."

>>>

>>> "... this call to @code{preadv2} or @code{pwritev2} ..." It does work

>>> in both, right?

>>>

>>> zw

>>>

>>

>> That's the new version:

>>

>> 	* sysdeps/unix/sysv/linux/bits/uio-ext.h (RWF_NOWAIT): New flag.

> 

>> diff --git a/sysdeps/unix/sysv/linux/bits/uio-ext.h b/sysdeps/unix/sysv/linux/bits/uio-ext.h

>> index 4ddb644..12cd78e 100644

>> --- a/sysdeps/unix/sysv/linux/bits/uio-ext.h

>> +++ b/sysdeps/unix/sysv/linux/bits/uio-ext.h

>> @@ -46,6 +46,10 @@ extern ssize_t process_vm_writev (pid_t __pid, const struct iovec *__lvec,

>>  #define RWF_HIPRI	0x00000001 /* High priority request.  */

>>  #define RWF_DSYNC	0x00000002 /* per-IO O_DSYNC.  */

>>  #define RWF_SYNC	0x00000004 /* per-IO O_SYNC.  */

>> +#define RWF_NOWAIT	0x00000008 /* per-IO, return -EAGAIN if operation

>> +				      would block */

>> +#define RWF_SUPPORTED	(RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT)

> 

> In case you haven't noticed already, this still defines RWF_SUPPORTED in

> uio-ext.h.

> 


Ugh, I will fix it, thanks for spotting it.
diff mbox

Patch

diff --git a/manual/llio.texi b/manual/llio.texi
index ba1f455..1aab522 100644
--- a/manual/llio.texi
+++ b/manual/llio.texi
@@ -770,6 +770,12 @@  Per-IO synchronization as if the file was opened with @code{O_DSYNC} flag.
 
 @item RWF_SYNC
 Per-IO synchronization as if the file was opened with @code{O_SYNC} flag.
+
+@item RWF_NOWAIT
+Set @code{preadv2} to return -EAGAIN if operation would block.
+
+@item RWF_SUPPORTED
+Mask with all supported flags.
 @end vtable
 
 When the source file is compiled with @code{_FILE_OFFSET_BITS == 64} the
diff --git a/misc/tst-preadvwritev2-common.c b/misc/tst-preadvwritev2-common.c
index 4c53d56..91cd58d 100644
--- a/misc/tst-preadvwritev2-common.c
+++ b/misc/tst-preadvwritev2-common.c
@@ -16,21 +16,15 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <limits.h>
 #include <support/check.h>
 
 static void
 do_test_with_invalid_flags (void)
 {
-  int invalid_flag = 0x1;
-#ifdef RWF_HIPRI
-  invalid_flag <<= 1;
-#endif
-#ifdef RWF_DSYNC
-  invalid_flag <<= 1;
-#endif
-#ifdef RWF_SYNC
-  invalid_flag <<= 1;
-#endif
+  /* Set the next bit from the mask of all supported flags.  */
+  int invalid_flag = __builtin_clz (RWF_SUPPORTED);
+  invalid_flag = 0x1 << ((sizeof (int) * CHAR_BIT) - invalid_flag);
 
   char buf[32];
   const struct iovec vec = { .iov_base = buf, .iov_len = sizeof (buf) };
diff --git a/sysdeps/unix/sysv/linux/bits/uio-ext.h b/sysdeps/unix/sysv/linux/bits/uio-ext.h
index 4ddb644..12cd78e 100644
--- a/sysdeps/unix/sysv/linux/bits/uio-ext.h
+++ b/sysdeps/unix/sysv/linux/bits/uio-ext.h
@@ -46,6 +46,10 @@  extern ssize_t process_vm_writev (pid_t __pid, const struct iovec *__lvec,
 #define RWF_HIPRI	0x00000001 /* High priority request.  */
 #define RWF_DSYNC	0x00000002 /* per-IO O_DSYNC.  */
 #define RWF_SYNC	0x00000004 /* per-IO O_SYNC.  */
+#define RWF_NOWAIT	0x00000008 /* per-IO, return -EAGAIN if operation
+				      would block */
+#define RWF_SUPPORTED	(RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT)
+
 
 __END_DECLS