linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Willy Tarreau <w@1wt.eu>
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Miguel Ojeda <ojeda@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	rust-for-linux@vger.kernel.org,
	Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Alex Gaynor <alex.gaynor@gmail.com>,
	Geoffrey Thomas <geofft@ldpreload.com>,
	Finn Behrens <me@kloenk.de>,
	Adam Bratschi-Kaye <ark.email@gmail.com>,
	Wedson Almeida Filho <wedsonaf@google.com>,
	Michael Ellerman <mpe@ellerman.id.au>
Subject: Re: [PATCH 04/13] Kbuild: Rust support
Date: Fri, 16 Apr 2021 22:22:15 +0200	[thread overview]
Message-ID: <20210416202215.GA11236@1wt.eu> (raw)
In-Reply-To: <CANiq72=RLf0FiuLVL-ZeLFp9P2LxTymbzhXoyQGG=tvUY_J-Sg@mail.gmail.com>

On Fri, Apr 16, 2021 at 08:57:07PM +0200, Miguel Ojeda wrote:
> On Fri, Apr 16, 2021 at 8:10 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > How well would ? operator fit that pattern?  _If_ it's just a syntax sugar
> > along the lines of "if argument matches Err(_), return Err(_)", the types
> > shouldn't be an issue, but that might need some fun with releasing resources,
> > etc.  If it's something more elaborate... details, please.
> 
> Yes, it is just syntax sugar -- it doesn't introduce any power to the language.
> 
> It was introduced because it is a very common pattern when using the
> `Result` and `Option` enums. In fact, before it existed, it was just a
> simple macro that you could also implement yourself.
> 
> For instance, given `Foo` and `Bar` types that need RAII cleanup of
> some kind (let's say `kill_foo()` and `kill_bar()`):
> 
>     fn foo() -> KernelResult<Foo> {
>         if black_box() {
>             return Err(EINVAL);
>         }
> 
>         // something that gets you a `Foo`
>         let foo = ...;
> 
>         Ok(foo)
>     }
> 
>     fn bar() -> KernelResult<Bar> {
>         let p = foo()?;
> 
>         // something that gets you a `Bar`, possibly using the `p`
>         let bar = ...;
> 
>         Ok(bar)
>     }
> 
> This reduces to (full example at https://godbolt.org/z/hjTxd3oP1):
> 
>     bar:
>             push    rbx
>             mov     ebx, 1
>             call    qword ptr [rip + black_box@GOTPCREL]
>             test    al, al
>             jne     .LBB2_2
>             call    qword ptr [rip + kill_foo@GOTPCREL]
>             xor     ebx, ebx
>     .LBB2_2:
>             mov     eax, ebx
>             mov     edx, -1234
>             pop     rbx
>             ret
> 
> You can see `bar()` calls `black_box()`. If it failed, it returns the
> EINVAL. Otherwise, it cleans up the `foo` automatically and returns
> the successful `bar`.

So it simply does the equivalent of:

  #define EINVAL -1234

  struct result {
     int status;
     int error;
  };

  extern bool black_box();
  extern void kill_foo();

  struct result bar()
  {
     return (struct error){ !!black_box(), EINVAL };
  }

  struct result foo()
  {
     struct result res = bar();

     if (res.status)
        goto leave;
     /* ... */
     kill_foo();   // only for rust, C doesn't need it
  leave:
     return res;
  }

So it simply returns a pair of values instead of a single one, which
is nothing special but not much conventional in the kernel either given
that ultimately syscalls will usually return a single value anyway. At
some point some code will have to remerge them to provide a composite
result and even take care of ambigous special cases like { true, 0 }
which may or may not indicate an error, and avoiding to consider the
unused error code on success.

For example some code called from mmap() might have to deal with this
this way:

   if (result.status == (void *)-1)
       return -result.error;
   else
       return result.status;

But possibly a lot of code feeding this result struct would be tempted
to do something like this to deal with NULL returns:

   result.status = foo_alloc();
   if (!result.status) {
       result.error = -ENOMEM;
       return result;
   }

And suddenly the error is lost, as a NULL is returned to the upper layers
which does not correspond to an failure status. Conversely, with a unique
result you'd do something like this:

   result = foo_alloc();
   if (!result)
       return -ENOMEM;

So it might possibly be safer to stick to the usually expected return
values instead of introducing composite ones.

I tend to agree that composite results can be better from new projects
started from scratch when all the API follows this principle, but here
there's quite a massive code base that was not designed along these
lines and I easily see how this can become a source of trouble over
time.

Willy

  reply	other threads:[~2021-04-16 20:22 UTC|newest]

Thread overview: 197+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-14 18:45 [PATCH 00/13] [RFC] " ojeda
2021-04-14 18:45 ` [PATCH 01/13] kallsyms: Support "big" kernel symbols (2-byte lengths) ojeda
2021-04-14 19:44   ` Matthew Wilcox
2021-04-14 19:59     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 02/13] kallsyms: Increase maximum kernel symbol length to 512 ojeda
2021-04-14 23:48   ` Nick Desaulniers
2021-04-14 18:45 ` [PATCH 03/13] Makefile: Generate CLANG_FLAGS even in GCC builds ojeda
2021-04-14 18:59   ` Nathan Chancellor
2021-04-15 10:18     ` Miguel Ojeda
2021-04-14 23:46   ` Nick Desaulniers
2021-04-15  0:47     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 04/13] Kbuild: Rust support ojeda
2021-04-14 23:19   ` Nick Desaulniers
2021-04-15  0:43     ` Miguel Ojeda
2021-04-15 18:03       ` Nick Desaulniers
2021-04-16 12:23         ` Miguel Ojeda
2021-04-17 19:35       ` Masahiro Yamada
2021-04-16 13:38   ` Peter Zijlstra
2021-04-16 17:05     ` Linus Torvalds
2021-04-16 17:47       ` Miguel Ojeda
2021-04-16 18:09         ` Al Viro
2021-04-16 18:57           ` Miguel Ojeda
2021-04-16 20:22             ` Willy Tarreau [this message]
2021-04-16 20:34               ` Connor Kuehl
2021-04-16 20:58                 ` Willy Tarreau
2021-04-16 21:39                   ` Miguel Ojeda
2021-04-16 22:04                     ` Willy Tarreau
2021-04-16 22:45                       ` Al Viro
2021-04-16 23:46                       ` Miguel Ojeda
2021-04-17  4:24                         ` Willy Tarreau
2021-04-17 15:38                           ` Miguel Ojeda
2021-04-16 21:19               ` Miguel Ojeda
2021-04-16 17:34     ` Miguel Ojeda
2021-04-19 19:58       ` David Sterba
2021-04-19 20:17         ` Matthew Wilcox
2021-04-19 21:03           ` Miguel Ojeda
2021-04-19 20:54         ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 05/13] Rust: Compiler builtins crate ojeda
2021-04-14 19:19   ` Linus Torvalds
2021-04-14 19:34     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 06/13] Rust: Module crate ojeda
2021-04-14 18:45 ` [PATCH 07/13] Rust: Kernel crate ojeda
2021-04-14 19:31   ` Linus Torvalds
2021-04-14 19:50     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 08/13] Rust: Export generated symbols ojeda
2021-04-14 18:46 ` [PATCH 09/13] Samples: Rust examples ojeda
2021-04-14 19:34   ` Linus Torvalds
2021-04-14 19:42     ` Miguel Ojeda
2021-04-14 19:49       ` Matthew Wilcox
2021-04-16 11:46       ` Andrej Shadura
2021-04-14 23:24     ` Nick Desaulniers
2021-04-15  7:10       ` Greg Kroah-Hartman
2021-04-15  7:39         ` Nick Desaulniers
2021-04-15 12:42         ` Miguel Ojeda
2021-04-16 13:07         ` Sven Van Asbroeck
2021-04-16 13:20           ` Greg Kroah-Hartman
2021-04-14 18:46 ` [PATCH 10/13] Documentation: Rust general information ojeda
2021-04-14 22:17   ` Nick Desaulniers
2021-04-14 23:34     ` Miguel Ojeda
2021-04-14 18:46 ` [PATCH 11/13] MAINTAINERS: Rust ojeda
2021-04-14 21:55   ` Nick Desaulniers
2021-04-14 22:02     ` Miguel Ojeda
2021-04-14 22:36   ` Nick Desaulniers
2021-04-14 18:46 ` [PATCH 12/13] Rust: add abstractions for Binder (WIP) ojeda
2021-04-14 18:46 ` [PATCH 13/13] Android: Binder IPC in Rust (WIP) ojeda
2021-04-14 19:44 ` [PATCH 00/13] [RFC] Rust support Linus Torvalds
2021-04-14 20:20   ` Miguel Ojeda
2021-04-15  1:38     ` Kees Cook
2021-04-15  8:26       ` David Laight
2021-04-15 18:08         ` Kees Cook
2021-04-15 12:39       ` Miguel Ojeda
2021-04-14 20:09 ` Matthew Wilcox
2021-04-14 20:21   ` Linus Torvalds
2021-04-14 20:35     ` Josh Triplett
2021-04-14 22:08     ` David Laight
2021-04-14 20:29   ` Miguel Ojeda
2021-04-18 15:31   ` Wedson Almeida Filho
2021-04-15  0:22 ` Nick Desaulniers
2021-04-15 10:05   ` Miguel Ojeda
2021-04-15 18:58 ` Peter Zijlstra
2021-04-16  2:22   ` Wedson Almeida Filho
2021-04-16  4:25     ` Al Viro
2021-04-16  5:02       ` Wedson Almeida Filho
2021-04-16  5:39         ` Paul Zimmerman
2021-04-16  7:46         ` Peter Zijlstra
2021-04-16  7:09     ` Peter Zijlstra
2021-04-17  5:23       ` comex
2021-04-17 12:46       ` David Laight
2021-04-17 14:51       ` Paolo Bonzini
2021-04-19  7:32         ` Peter Zijlstra
2021-04-19  7:53           ` Paolo Bonzini
2021-04-19  8:26             ` Peter Zijlstra
2021-04-19  8:35               ` Peter Zijlstra
2021-04-19  9:02               ` Paolo Bonzini
2021-04-19  9:36                 ` Peter Zijlstra
2021-04-19  9:40                   ` Paolo Bonzini
2021-04-19 11:01                     ` Will Deacon
2021-04-19 17:14                   ` Linus Torvalds
2021-04-19 18:38                     ` Paolo Bonzini
2021-04-19 18:50                       ` Linus Torvalds
2021-04-22 10:03     ` Linus Walleij
2021-04-22 14:09       ` David Laight
2021-04-22 15:24       ` Wedson Almeida Filho
2021-04-26  0:18         ` Linus Walleij
2021-04-26 14:26           ` Miguel Ojeda
2021-04-26 14:40           ` Wedson Almeida Filho
2021-04-26 16:03             ` Miguel Ojeda
2021-04-27 10:54             ` Linus Walleij
2021-04-27 11:13               ` Robin Randhawa
2021-04-29  1:52               ` Wedson Almeida Filho
2021-04-26 18:01           ` Miguel Ojeda
2021-04-22 21:28       ` Miguel Ojeda
2021-04-26  0:31         ` Linus Walleij
2021-04-26 18:18           ` Miguel Ojeda
2021-04-27 11:13             ` Linus Walleij
2021-04-28  2:51               ` Kyle Strand
2021-04-28  3:10               ` Miguel Ojeda
2021-05-04 21:21                 ` Linus Walleij
2021-05-04 23:30                   ` Miguel Ojeda
2021-05-05 11:34                     ` Linus Walleij
2021-05-05 14:17                       ` Miguel Ojeda
2021-05-05 15:13                         ` Enrico Weigelt, metux IT consult
2021-05-06 12:47                         ` Linus Walleij
2021-05-07 18:23                           ` Miguel Ojeda
2021-04-16  4:27   ` Boqun Feng
2021-04-16  6:04     ` Nick Desaulniers
2021-04-16 18:47       ` Paul E. McKenney
2021-04-19 20:35         ` Nick Desaulniers
2021-04-19 21:37           ` Paul E. McKenney
2021-04-19 22:03           ` Miguel Ojeda
2021-04-16 20:48     ` Josh Triplett
2021-04-16  8:16   ` Michal Kubecek
2021-04-16  9:29     ` Willy Tarreau
2021-04-16 11:24 ` Peter Zijlstra
2021-04-16 13:07   ` Wedson Almeida Filho
2021-04-16 14:19     ` Peter Zijlstra
2021-04-16 15:04       ` Miguel Ojeda
2021-04-16 15:43         ` Peter Zijlstra
2021-04-16 16:21           ` Miguel Ojeda
2021-04-16 15:33       ` Wedson Almeida Filho
2021-04-16 16:14         ` Willy Tarreau
2021-04-16 17:10           ` Miguel Ojeda
2021-04-16 17:18             ` Peter Zijlstra
2021-04-16 18:08               ` Matthew Wilcox
2021-04-17 11:17                 ` Peter Zijlstra
2021-04-17 11:46                   ` Willy Tarreau
2021-04-17 14:24                     ` Peter Zijlstra
2021-04-17 14:36                       ` Willy Tarreau
2021-04-17 13:46                   ` David Laight
2021-04-16 17:37             ` Willy Tarreau
2021-04-16 17:46               ` Connor Kuehl
2021-04-20  0:24               ` Nick Desaulniers
2021-04-20  3:47                 ` Willy Tarreau
2021-04-20  5:56                 ` Greg Kroah-Hartman
2021-04-20  6:16                   ` Willy Tarreau
2021-04-29 15:38                     ` peter enderborg
2021-04-17 13:53           ` Wedson Almeida Filho
2021-04-17 14:21             ` Willy Tarreau
2021-04-17 15:23               ` Miguel Ojeda
2021-04-18 15:51               ` Wedson Almeida Filho
2021-04-17 12:41       ` David Laight
2021-04-17 13:01         ` Wedson Almeida Filho
2021-04-16 15:03     ` Matthew Wilcox
2021-04-17 13:29       ` Wedson Almeida Filho
2021-04-16 15:58     ` Theodore Ts'o
2021-04-16 16:21       ` Wedson Almeida Filho
2021-04-17 15:11       ` Paolo Bonzini
2021-04-16 14:21   ` Miguel Ojeda
2021-04-17 20:42 ` Richard Weinberger
2021-04-28 18:34 ` Mariusz Ceier
2021-04-28 20:25   ` Nick Desaulniers
2021-04-28 21:21   ` David Laight
2021-04-29 11:14     ` Kajetan Puchalski
2021-04-29 11:25   ` Kajetan Puchalski
2021-04-29 14:06     ` Mariusz Ceier
2021-04-29 14:13       ` Sven Van Asbroeck
2021-04-29 14:26         ` Willy Tarreau
2021-04-29 15:06       ` Al Viro
2021-04-29 16:09         ` Mariusz Ceier
2021-04-30  6:39     ` Thomas Schoebel-Theuer
2021-04-30  8:30       ` David Laight
2021-05-05 13:58       ` Enrico Weigelt, metux IT consult
2021-05-05 14:41         ` Miguel Ojeda
2022-06-20 15:11 ` Olliver Schinagl
2022-06-27 17:44   ` Miguel Ojeda
2022-07-18  6:56     ` Olliver Schinagl
2022-07-20 19:23       ` Miguel Ojeda
2022-07-20 20:21         ` Nicolas Pitre
2022-07-27  7:47           ` Olliver Schinagl
2022-07-27 13:32             ` Nicolas Pitre
2022-07-27  8:05         ` Olliver Schinagl
2022-07-28 10:21           ` Gary Guo
2022-07-28 12:09             ` Greg Kroah-Hartman
2022-07-28 12:28               ` Gary Guo
2022-07-28 20:45               ` Olliver Schinagl
2022-07-29  8:04                 ` Greg Kroah-Hartman
2022-07-28 20:43             ` Olliver Schinagl

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210416202215.GA11236@1wt.eu \
    --to=w@1wt.eu \
    --cc=alex.gaynor@gmail.com \
    --cc=ark.email@gmail.com \
    --cc=geofft@ldpreload.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=me@kloenk.de \
    --cc=miguel.ojeda.sandonis@gmail.com \
    --cc=mpe@ellerman.id.au \
    --cc=ojeda@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=wedsonaf@google.com \
    --subject='Re: [PATCH 04/13] Kbuild: Rust support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).