Date:   Fri, 16 Apr 2021 22:22:15 +0200
From:   Willy Tarreau <w@....eu>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     Al Viro <viro@...iv.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Miguel Ojeda <ojeda@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        rust-for-linux@...r.kernel.org,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Geoffrey Thomas <geofft@...reload.com>,
        Finn Behrens <me@...enk.de>,
        Adam Bratschi-Kaye <ark.email@...il.com>,
        Wedson Almeida Filho <wedsonaf@...gle.com>,
        Michael Ellerman <mpe@...erman.id.au>
Subject: Re: [PATCH 04/13] Kbuild: Rust support

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@...iv.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@...PCREL]
>             test    al, al
>             jne     .LBB2_2
>             call    qword ptr [rip + kill_foo@...PCREL]
>             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

Powered by blists - more mailing lists