Re: [RFC 10/18] rust: split "qom" crate

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3965 bytes --]

Hi

On Wed, Aug 27, 2025 at 10:33 AM Zhao Liu <zhao1.liu@intel.com> wrote:

> > diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/
> device.rs
> > index 7cffb894a8..a3bcd1297a 100644
> > --- a/rust/hw/char/pl011/src/device.rs
> > +++ b/rust/hw/char/pl011/src/device.rs
> > @@ -21,10 +21,13 @@
> >      memory::{hwaddr, MemoryRegion, MemoryRegionOps,
> MemoryRegionOpsBuilder},
> >      prelude::*,
> >      qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property,
> ResetType, ResettablePhasesImpl},
> > -    qom::{ObjectImpl, Owned, ParentField, ParentInit},
> >      sysbus::{SysBusDevice, SysBusDeviceImpl},
> >      vmstate_clock,
> >  };
> > +use qom::{
> > +    qom_isa, IsA, Object, ObjectClassMethods, ObjectDeref, ObjectImpl,
> ObjectMethods, ObjectType,
> > +    Owned, ParentField, ParentInit,
> > +};
>
> These QOM parts are frequently used and very common. at least for qom,
> I think prelude would help a lot.


ack


> A qom prelude could help reduce the changes in other parts (pl011/
> hpet/memory...).
>
> > diff --git a/rust/qom/meson.build b/rust/qom/meson.build
> > new file mode 100644
> > index 0000000000..6e95d75fa0
> > --- /dev/null
> > +++ b/rust/qom/meson.build
> > @@ -0,0 +1,61 @@
> > +_qom_cfg = run_command(rustc_args,
> > +  '--config-headers', config_host_h, '--features', files('Cargo.toml'),
> > +  capture: true, check: true).stdout().strip().splitlines()
> > +
> > +# TODO: Remove this comment when the clang/libclang mismatch issue is
> solved.
> > +#
> > +# Rust bindings generation with `bindgen` might fail in some cases
> where the
> > +# detected `libclang` does not match the expected `clang`
> version/target. In
> > +# this case you must pass the path to `clang` and `libclang` to your
> build
> > +# command invocation using the environment variables CLANG_PATH and
> > +# LIBCLANG_PATH
> > +_qom_bindings_inc_rs = rust.bindgen(
> > +  input: 'wrapper.h',
> > +  dependencies: common_ss.all_dependencies(),
> > +  output: 'bindings.inc.rs',
>
> There're many binding files with the same name. What about adding a prefix
> like "qom-bindings" to distinguish it? This can help search and locate
> specific binding file.
>

they are already under different directories :)


>
> > +  include_directories: bindings_incdir,
> > +  bindgen_version: ['>=0.60.0'],
> > +  args: bindgen_args_common,
> > +)
>
> ...
>
> > diff --git a/rust/qom/tests/tests.rs b/rust/qom/tests/tests.rs
> > new file mode 100644
> > index 0000000000..49f1cbecf5
> > --- /dev/null
> > +++ b/rust/qom/tests/tests.rs
> > @@ -0,0 +1,47 @@
> > +use std::{ffi::CStr, sync::LazyLock};
>
> LazyLock is useful, but it became stable since v1.80. So if Paolo
> decide pick this series after v1.83 support, it's fine.
>

ah, right. I wonder why rust-version didn't warn me about this. I'll check

>
> > +use qom::{qom_isa, Object, ObjectClassMethods, ObjectImpl, ObjectType,
> ParentField};
> > +use util::bindings::{module_call_init, module_init_type};
>
> ...
>
> > +fn init_qom() {
> > +    static ONCE: LazyLock<()> = LazyLock::new(|| unsafe {
> > +        module_call_init(module_init_type::MODULE_INIT_QOM);
> > +    });
>
> But for now, we can still use a static BqlCell<bool> as the workaround,
> just like rust/hwcore/tests/tests.rs did, to decouple with MSRV
> dependency.
>
> And it seems rust/hwcore/tests/tests.rs has already covered this test
> case, do we still need this test?
>

I wanted a simpler test that focuses on QOM only. But this may not be
desirable after all.


>
> If so, then it's better to add this new test in a seperate patch, which
> makes current patch focus on the splitting :-).
>

Agree, I'll drop it for now.


>
> > +    bql::start_test();
> > +    LazyLock::force(&ONCE);
> > +}
> > +
>
> Otherwise, LGTM,
>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>
>

[-- Attachment #2: Type: text/html, Size: 6281 bytes --]