| Message ID | 20250211214328.640374-4-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust/block: Add minimal block driver bindings | expand |
On 2/11/25 22:43, Kevin Wolf wrote: > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > rust/wrapper.h | 4 ++++ > meson.build | 1 + > rust/qemu-api/src/zeroable.rs | 5 +++-- > 3 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/rust/wrapper.h b/rust/wrapper.h > index 41be87adcf..c3e1e6f9cf 100644 > --- a/rust/wrapper.h > +++ b/rust/wrapper.h > @@ -53,3 +53,7 @@ typedef enum memory_order { > #include "chardev/char-fe.h" > #include "qapi/error.h" > #include "chardev/char-serial.h" > +#include "block/block.h" > +#include "block/block_int.h" > +#include "block/qdict.h" > +#include "qapi/qapi-visit-block-core.h" > diff --git a/meson.build b/meson.build > index 30aae6b3c3..154195bc80 100644 > --- a/meson.build > +++ b/meson.build > @@ -4045,6 +4045,7 @@ if have_rust > '--with-derive-default', > '--no-layout-tests', > '--no-prepend-enum-name', > + '--allowlist-item', 'EINVAL|EIO', I've got some errno bindings that I wrote for chardev, I'll send them shortly. Paolo > '--allowlist-file', meson.project_source_root() + '/include/.*', > '--allowlist-file', meson.project_source_root() + '/.*', > '--allowlist-file', meson.project_build_root() + '/.*' > diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs > index b454e9e05e..60af681293 100644 > --- a/rust/qemu-api/src/zeroable.rs > +++ b/rust/qemu-api/src/zeroable.rs > @@ -56,7 +56,6 @@ pub unsafe trait Zeroable: Default { > /// ## Differences with `core::mem::zeroed` > /// > /// `const_zero` zeroes padding bits, while `core::mem::zeroed` doesn't > -#[allow(unused)] > macro_rules! const_zero { > // This macro to produce a type-generic zero constant is taken from the > // const_zero crate (v0.1.1): > @@ -78,7 +77,6 @@ union TypeAsBytes { > } > > /// A wrapper to implement the `Zeroable` trait through the `const_zero` macro. > -#[allow(unused)] > macro_rules! impl_zeroable { > ($type:ty) => { > unsafe impl Zeroable for $type { > @@ -110,3 +108,6 @@ fn default() -> Self { > impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1); > #[cfg(feature = "system")] > impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2); > + > +impl_zeroable!(crate::bindings::BlockDriver); > +impl_zeroable!(crate::bindings::BlockDriver__bindgen_ty_1);
Am 12.02.2025 um 10:29 hat Paolo Bonzini geschrieben: > On 2/11/25 22:43, Kevin Wolf wrote: > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > rust/wrapper.h | 4 ++++ > > meson.build | 1 + > > rust/qemu-api/src/zeroable.rs | 5 +++-- > > 3 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/rust/wrapper.h b/rust/wrapper.h > > index 41be87adcf..c3e1e6f9cf 100644 > > --- a/rust/wrapper.h > > +++ b/rust/wrapper.h > > @@ -53,3 +53,7 @@ typedef enum memory_order { > > #include "chardev/char-fe.h" > > #include "qapi/error.h" > > #include "chardev/char-serial.h" > > +#include "block/block.h" > > +#include "block/block_int.h" > > +#include "block/qdict.h" > > +#include "qapi/qapi-visit-block-core.h" > > diff --git a/meson.build b/meson.build > > index 30aae6b3c3..154195bc80 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -4045,6 +4045,7 @@ if have_rust > > '--with-derive-default', > > '--no-layout-tests', > > '--no-prepend-enum-name', > > + '--allowlist-item', 'EINVAL|EIO', > > I've got some errno bindings that I wrote for chardev, I'll send them > shortly. 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)) Which actually already shows that your errno binding patch does the opposite direction of what I needed in this series. My problem is when I need to return an int to C, and I either have an io::Result or I just want to directly return an errno value. So we'll have to add that part to your errno module, too. Kevin
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... > 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. 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. Paolo > My problem is when I > need to return an int to C, and I either have an io::Result or I just > want to directly return an errno value. So we'll have to add that part > to your errno module, too.
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
On 2/12/25 16:13, Kevin Wolf wrote: > 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. Or just include the libc crate, see attachment. Paolo
Am 12.02.2025 um 18:16 hat Paolo Bonzini geschrieben: > On 2/12/25 16:13, Kevin Wolf wrote: > > 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. > > Or just include the libc crate, see attachment. I assume that sooner or later we'll have a reason to include it anyway, so that might honestly be the best option. Do you want to post it as a proper patch? It seems to depend on your errno patch, but that shouldn't be a problem because we still want that one, too. Kevin
On Wed, Feb 12, 2025 at 8:52 PM Kevin Wolf <kwolf@redhat.com> wrote: > I assume that sooner or later we'll have a reason to include it anyway, > so that might honestly be the best option. Sounds good. > Do you want to post it as a proper patch? It seems to depend on your > errno patch, but that shouldn't be a problem because we still want that > one, too. Yes, I wrote it as an addendum, as a kind of "let's see later what's best"; that's also why the patch I posted uses "mod libc" for the errno values. (The attachment gives away the date, too :)). I'll clean up everything and add errno::into_neg_errno(); it should take both an unsigned Ok value or Ok(()), and will return i32 or i64. Paolo
diff --git a/rust/wrapper.h b/rust/wrapper.h index 41be87adcf..c3e1e6f9cf 100644 --- a/rust/wrapper.h +++ b/rust/wrapper.h @@ -53,3 +53,7 @@ typedef enum memory_order { #include "chardev/char-fe.h" #include "qapi/error.h" #include "chardev/char-serial.h" +#include "block/block.h" +#include "block/block_int.h" +#include "block/qdict.h" +#include "qapi/qapi-visit-block-core.h" diff --git a/meson.build b/meson.build index 30aae6b3c3..154195bc80 100644 --- a/meson.build +++ b/meson.build @@ -4045,6 +4045,7 @@ if have_rust '--with-derive-default', '--no-layout-tests', '--no-prepend-enum-name', + '--allowlist-item', 'EINVAL|EIO', '--allowlist-file', meson.project_source_root() + '/include/.*', '--allowlist-file', meson.project_source_root() + '/.*', '--allowlist-file', meson.project_build_root() + '/.*' diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs index b454e9e05e..60af681293 100644 --- a/rust/qemu-api/src/zeroable.rs +++ b/rust/qemu-api/src/zeroable.rs @@ -56,7 +56,6 @@ pub unsafe trait Zeroable: Default { /// ## Differences with `core::mem::zeroed` /// /// `const_zero` zeroes padding bits, while `core::mem::zeroed` doesn't -#[allow(unused)] macro_rules! const_zero { // This macro to produce a type-generic zero constant is taken from the // const_zero crate (v0.1.1): @@ -78,7 +77,6 @@ union TypeAsBytes { } /// A wrapper to implement the `Zeroable` trait through the `const_zero` macro. -#[allow(unused)] macro_rules! impl_zeroable { ($type:ty) => { unsafe impl Zeroable for $type { @@ -110,3 +108,6 @@ fn default() -> Self { impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1); #[cfg(feature = "system")] impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2); + +impl_zeroable!(crate::bindings::BlockDriver); +impl_zeroable!(crate::bindings::BlockDriver__bindgen_ty_1);
Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- rust/wrapper.h | 4 ++++ meson.build | 1 + rust/qemu-api/src/zeroable.rs | 5 +++-- 3 files changed, 8 insertions(+), 2 deletions(-)