Re: [PATCH 03/11] rust: Add some block layer bindings

[Kevin Wolf]

Am 12.02.2025 um 14:47 hat Paolo Bonzini geschrieben:
> On Wed, Feb 12, 2025 at 2:13 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > Yes, we definitely need some proper bindings there. I'm already tired of
> > writing things like this:
> >
> >     return -(bindings::EINVAL as std::os::raw::c_int)
> >
> > Or even:
> >
> >     return e
> >         .raw_os_error()
> >         .unwrap_or(-(bindings::EIO as std::os::raw::c_int))
> 
> This by the way seemed incorrect to me as it should be
> 
>      return -(e
>          .raw_os_error()
>          .unwrap_or(bindings::EIO as std::os::raw::c_int))
> 
> (leaving aside that raw_os_error does not work on Windows)... But then
> I noticed that read_raw() also does not negate, which causes the error
> to print incorrectly...

Yes, I actually noticed that after sending the email and fixed it (and
another instances of the same) up locally.

> > Which actually already shows that your errno binding patch does the
> > opposite direction of what I needed in this series.
> 
> ... so my patch already helps a bit: you can still change
> 
>     if ret < 0 {
>          Err(Error::from_raw_os_error(ret))
>     } else {
>          Ok(())
>     }
> 
> to
> 
>    errno::into_io_result(ret)?;
>    Ok(())
> 
> and avoid the positive/negative confusion.

No reason to write essentially if (ret != 0) { ret } else { 0 }. This
one should do:

    errno::into_io_result(ret)

And yes, it's already a good improvement.

> Anyhow, I guess the first one wouldn't be much better:
> 
>    return errno::into_negative_errno(ErrorKind::InvalidInput);
> 
> whereas the second could be
> 
>    return errno::into_negative_errno(e);
> 
> But then the first is already a special case; it only happens where
> your bindings are not trivial thanks to the introduction of the
> Mapping type.

Yes, the second one seems like the more important one because the other
one should only happen in bindings eventually. We could still have
something like an errno!(InvalidInput) to make the code in bindings less
verbose.

Or if you have to define the constants anyway - you currently do this
only for Windows, but for into_negative_errno() you might need it on
Linux, too - and it wouldn't be a problem for the constants to be
signed (that they are unsigned is the main reason why it becomes so ugly
with the bindgen constants), you could just make it -errno::EINVAL
again.

Kevin