linux-kernel - Re: [PATCH 24/24] Documentation: document ioctl interfaces better

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Dec 2019 14:05:51 -0700
From:   Jonathan Corbet <corbet@....net>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Jens Axboe <axboe@...nel.dk>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-kernel@...r.kernel.org, y2038@...ts.linaro.org,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Jonathan Neuschäfer <j.neuschaefer@....net>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Kent Overstreet <kent.overstreet@...il.com>,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH 24/24] Documentation: document ioctl interfaces better

On Wed, 11 Dec 2019 21:42:58 +0100
Arnd Bergmann <arnd@...db.de> wrote:

> Documentation/process/botching-up-ioctls.rst was orignally
> written as a blog post for DRM driver writers, so it it misses
> some points while going into a lot of detail on others.
> 
> Try to provide a replacement that addresses typical issues
> across a wider range of subsystems, and follows the style of
> the core-api documentation better.
> 
> Signed-off-by: Arnd Bergmann <arnd@...db.de>

Thanks for improving the docs!  I have a few nits outside of the content
itself.

>  Documentation/core-api/index.rst |   1 +
>  Documentation/core-api/ioctl.rst | 250 +++++++++++++++++++++++++++++++
>  2 files changed, 251 insertions(+)
>  create mode 100644 Documentation/core-api/ioctl.rst

So you left the old document in place; was that intentional?

> diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
> index ab0eae1c153a..3f28b2f668be 100644
> --- a/Documentation/core-api/index.rst
> +++ b/Documentation/core-api/index.rst
> @@ -39,6 +39,7 @@ Core utilities
>     ../RCU/index
>     gcc-plugins
>     symbol-namespaces
> +   ioctl
>  
>  
>  Interfaces for kernel debugging
> diff --git a/Documentation/core-api/ioctl.rst b/Documentation/core-api/ioctl.rst
> new file mode 100644
> index 000000000000..cb2c86ae63e7
> --- /dev/null
> +++ b/Documentation/core-api/ioctl.rst
> @@ -0,0 +1,250 @@
> +======================
> +ioctl based interfaces
> +======================
> +
> +:c:func:`ioctl` is the most common way for applications to interface

Please don't use :c:func: anymore.  If you just say "ioctl()" the right
thing will happen (which is nothing here, since there isn't anything that
makes sense to link to in the internal kernel context).

We need a checkpatch rule for :c:func: I guess.

Similarly, later on you have:

> +Timeout values and timestamps should ideally use CLOCK_MONOTONIC time,
> +as returned by ``ktime_get_ns()`` or ``ktime_get_ts64()``.  Unlike
> +CLOCK_REALTIME, this makes the timestamps immune from jumping backwards
> +or forwards due to leap second adjustments and clock_settime() calls.

Making those functions ``literal`` will defeat the automatic
cross-referencing.  Better to just say ktime_get_ns() without quotes.

[...]

> +* On the x86-32 (i386) architecture, the alignment of 64-bit variables
> +  is only 32 bit, but they are naturally aligned on most other
> +  architectures including x86-64. This means a structure like
> +
> +  ::

You don't need the extra lines here; just say "...a structure like::"

> +    struct foo {
> +        __u32 a;
> +        __u64 b;
> +        __u32 c;
> +    };
> +

Thanks,

jon