Hello Alex,

On 11/20/20 4:03 PM, Alejandro Colomar (man-pages) wrote:

Hi Omar,

I found a wording of mine to be a bit confusing.
Please see below.

Thanks,

Alex

On 11/20/20 3:06 PM, Alejandro Colomar (man-pages) wrote:

quoted

Hi Omar and Michael,

please, see below.

Thanks,

Alex

On 11/20/20 12:29 AM, Alejandro Colomar (mailing lists; readonly) wrote:

quoted

Hi Omar,

Please, see some fixes below:

Michael, I've also some questions for you below
(you can grep for mtk to find those).

Thanks,

Alex

On 11/18/20 8:18 PM, Omar Sandoval wrote:

quoted

From: Omar Sandoval <redacted>

This adds a new page, encoded_io(7), providing an overview of encoded
I/O and updates fcntl(2), open(2), and preadv2(2)/pwritev2(2) to
reference it.

Cc: Michael Kerrisk <redacted>
Cc: linux-man <redacted>
Signed-off-by: Omar Sandoval <redacted>
---
This feature is not yet upstream.

 man2/fcntl.2      |  10 +-
 man2/open.2       |  23 +++
 man2/readv.2      |  70 +++++++++
 man7/encoded_io.7 | 369 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 471 insertions(+), 1 deletion(-)
 create mode 100644 man7/encoded_io.7

diff --git a/man2/fcntl.2 b/man2/fcntl.2
index 546016617..b0d7fa2c3 100644
--- a/man2/fcntl.2
+++ b/man2/fcntl.2

@@ -221,8 +221,9 @@ On Linux, this command can change only the

[...]

quoted

quoted

quoted

+.PP
+This may be extended in the future, so
+.I iov[0].iov_len
+must be set to
+.I "sizeof(struct\ encoded_iov)"
+for forward/backward compatibility.
+The remaining buffers contain the encoded data.
+.PP
+.I compression
+and
+.I encryption
+are the encoding fields.
+.I compression
+is
+.B ENCODED_IOV_COMPRESSION_NONE
+(zero)
+or a filesystem-specific
+.B ENCODED_IOV_COMPRESSION

Maybe s/ENCODED_IOV_COMPRESSION/ENCODED_IOV_COMPRESSION_*/

Or s/ENCODED_IOV_COMPRESSION/ENCODED_IOV_COMPRESSION_/

I'm not sure about existing practice.

Michael (mtk), what would you do here?

I think I've tended towards the former
(ENCODED_IOV_COMPRESSION_*) in the past.

quoted

quoted

quoted

+constant;
+see
+.BR Filesystem\ support .

Please, write it as [.BR "Filesystem support" .]

and maybe I would change it, to be more specific, to the following:

[
see
.B Filesystem support
below.
]

So that the reader clearly understands it's on the same page.

quoted

+.I encryption
+is currently always
+.B ENCODED_IOV_ENCRYPTION_NONE
+(zero).
+.PP
+.I unencoded_len
+is the length of the unencoded (i.e., decrypted and decompressed) data.
+.I unencoded_offset
+is the offset into the unencoded data where the data in the file begins

The above wording is a bit unclear to me.

I suggest the following:

[
.I unencoded_offset
is the offset from the begining of the file
to the first byte of the unencoded data
]

Now I've read it again, and my wording was even worse than yours.
I think yours can be understood after a few reads.

However, I'll still try to reword mine to see if I add some value:

[
.I unencoded_offset
is the offset from the first byte of the unencoded data
to the first byte of logical data.
]

If you prefer yours, or a mix, that's fine.

quoted

quoted

quoted

+(less than or equal to
+.IR unencoded_len ).
+.I len
+is the length of the data in the file
+(less than or equal to
+.I unencoded_len
+-

Here's a question for Michael (mtk):

I've seen (many) cases where these math operations
are written without spaces,
and in the same line (e.g., [.IR a + b]).

I'd like to know your preferences on this,
or what is actually more extended in the manual pages,
to stick with only one of them.

I suspect there's a lot of inconsistency across pages. For simple
cases like this, I think writing it without spaces is fine, and
perhaps even preferable.

quoted

quoted

quoted

+.IR unencoded_offset ).
+See
+.B Extent layout
+below for some examples.
+.I

Were you maybe going to add something there?

If not, please remove that [.I].

quoted

+.PP
+If the unencoded data is actually longer than
+.IR unencoded_len ,
+then it is truncated;
+if it is shorter, then it is extended with zeroes.
+.PP
+

Please, remove that blank line.

quoted

+.BR pwritev2 ()

Should be [.BR pwritev2 (2)]

Michael (mtk),

Am I right in that?  Please, confirm.

Yes. References to functions documented in other pages should
include the section number in parentheses.

[...]

quoted

quoted

quoted

+.PP
+However, suppose we read 50 bytes into a file
+which contains a single compressed extent.
+The filesystem must still return the entire compressed extent
+for us to be able to decompress it,
+so
+.I unencoded_len
+would be the length of the entire decompressed extent.
+However, because the read was at offset 50,
+the first 50 bytes should be ignored.
+Therefore,
+.I unencoded_offset
+would be 50,
+and
+.I len
+would accordingly be
+.IR unencoded_len\ -\ 50 .

This formats everything as I, except for the last dot.
Replace by:

[
.I unencoded
- 50.
]

Michael (mtk), same as above:
to space, or not to space?  That is the question :p

In this case, perhaps

.IR unencoded \-1

[...]

quoted

quoted

quoted

+.SS Security
+Encoded I/O creates the potential for some security issues:
+.IP * 3
+Encoded writes allow writing arbitrary data which the kernel will decode on
+a subsequent read. Decompression algorithms are complex and may have bugs
+which can be exploited by maliciously crafted data.
+.IP *
+Encoded reads may return data which is not logically present in the file
+(see the discussion of
+.I len
+vs.

Please, s/vs./vs/
See the reasons below:

Michael (mtk),

Here the renderer outputs a double space
(as for separating two sentences).

Are you okay with that?

Yes, that should probably be avoided. I'm not sure what the
correct way is to prevent that in groff though. I mean, one
could write

.RI "vs.\ " unencoded_len

but I think that simply creates a nonbreaking space,
which is not exactly what is desired.

[....]

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/