linux-kernel - Re: [RFC PATCH 0/2] Generate API documentation for 'bindings' crate

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:   Thu, 3 Aug 2023 11:35:09 -0400
From:   Trevor Gross <tmgross@...ch.edu>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Benno Lossin <benno.lossin@...ton.me>,
        rust-for-linux@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/2] Generate API documentation for 'bindings' crate

On Thu, Aug 3, 2023 at 10:08 AM Miguel Ojeda
<miguel.ojeda.sandonis@...il.com> wrote:
> I think the first question to answer would be whether we want to
> expose `bindings`, i.e. what are the advantages/disadvantages?
>
> If `kernel` were a "normal library", then I would say we shouldn't,
> because users should not need to care; and, in addition, the goal is
> that leaf modules do not need to access them directly.
>
> But, as sometimes happen, it may still be quite useful for some
> developers nevertheless (the same way documenting the internal/private
> details could be).
>
> So, it would be nice to have an overview from your point of view on
> why it should be done (or not).

I do understand that dilemma, but am not sure what the happy medium is
between rendering them and hiding them. Where I see value is:

1. For anyone reading/writing abstractions, it's useful to quickly see
how exactly bindgen did the C -> Rust mapping
2. Abstractions may want to link to the C side somehow, linking the
bindings is an easier first step than linking to sphinx (in the future
we may be able to do a bindings -> sphinx link)

Maybe a stronger "prefer abstractions over bindings" message would
suffice to discourage use outside of reference?

In any case, I will put this behind a flag so it is not enabled by
default. While I'm at it - is there value in adding a config option to
pass `--document-private-items` to the kernel crate, or supporting
`RUSTDOCFLAGS` like Cargo does?

> > 1. Do we want to make this the default, or a separate target/
> >    configuration? I don't think there is much downside to always
> >    generating.
>
> One downside of doing it by default would be going against the "avoid
> `bindings`" guideline (ideally rule).
>
> Another one is render time (the C side is trying to reduce it), I
> guess, especially if we keep adding headers over time.

That makes sense, I will add the flag option.

> > 2. The entire '.config' file could be included in the doc output, to
> >    make it easy to tell what settings the documentation was generated
> >    with. Would this be desired? Maybe with a '--cfg
> >    include-dotcfg=".config"' flag so published docs would have the
> >    option (unsure whether it may ever have sensitive information).
>
> This may be useful orthogonally to rendering `bindings` or not.
>

I will add this in a separate patch.

> > Bindgen is currently invoked with '--no-doc-comments', I think this may
> > be because some blocks were mistakenly interpreted as doctests. Once we
> > upgrade our bindgen version we might be able to remove this.
>
> Yes, that is https://github.com/Rust-for-Linux/linux/issues/323 and
> https://github.com/rust-lang/rust-bindgen/issues/2057, which led to
> the addition of `process_comments` to `bindgen` in v0.63.0.

How would switching to the library work? Since that seems like a more
involved discussion, would postprocesing `generated_bindings.rs` be
acceptable instead? I have been playing around with a python script
that extracts the `#[doc ...]` blocks and (1) fixes the escaping and
(2) formats parameters and fixes their spacing, I could extract this
to a separate patch if it may be a while before we can use the
library.

> > Side note, 'rust/Makefile' seems to have a mix of tabs and spaces - is
> > this intentional?
>
> Yes, it is intentional. For instance, the command definitions use
> spaces like the vast majority of the kernel `Makefile`s.

Ah thanks, it just looks a bit weird in the diff.

> Cheers,
> Miguel

Thanks!
Trevor