| Message ID | 20250214135822.4076174-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: tests: do not import bindings::* | expand |
On Fri, Feb 14, 2025 at 02:58:22PM +0100, Paolo Bonzini wrote: > Similar to the devices, spell the exact set of C functions that are > called directly. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > rust/qemu-api/tests/tests.rs | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs > index 92dbfb8a0c8..03569e4a44c 100644 > --- a/rust/qemu-api/tests/tests.rs > +++ b/rust/qemu-api/tests/tests.rs > @@ -8,13 +8,14 @@ > }; > > use qemu_api::{ > - bindings::*, > + bindings::{module_call_init, module_init_type, object_new, object_unref, qdev_prop_bool}, > c_str, > cell::{self, BqlCell}, > declare_properties, define_property, > prelude::*, Should this be expanded too if we want to discourage wildcard imports ? > qdev::{DeviceClass, DeviceImpl, DeviceState, Property, ResettablePhasesImpl}, > qom::{ClassInitImpl, ObjectImpl, ParentField}, > + sysbus::SysBusDevice, This addition seems distinct from the bindings::* removal. Should it have been a separate patch ? > vmstate::VMStateDescription, > zeroable::Zeroable, > }; With regards, Daniel
First of all, thanks for taking a look!! > > prelude::*, > > Should this be expanded too if we want to discourage wildcard imports ? The prelude is a bit special; it provides a subset of items that most users will want to include into the namespace. You're right that it goes against the rule of discouraging wildcard imports, and in fact that's why it almost entirely consists of extension traits (e.g. DeviceMethods, SysBusDeviceMethods, ObjectCast, ObjectDeref, ObjectClassMethods). You shouldn't have to name the traits at all if the API works, and having them in the prelude lowers the cognitive load. There are a few more symbols that are imported in the prelude: for example the BqlCell and BqlRefCell, the ObjectType and IsA traits, or the qom_isa macro; those will be used almost everywhere. The exact shape of the prelude might change in the future, since qemu_api might get split into separate crates to enable using Rust in tools (which do not have qdev). For example there could be separate block, qdev and qom preludes. In any case, it's an exception and for this reason the amount of non-extension-traits symbols that it includes should be limited. > > qdev::{DeviceClass, DeviceImpl, DeviceState, Property, ResettablePhasesImpl}, > > qom::{ClassInitImpl, ObjectImpl, ParentField}, > > + sysbus::SysBusDevice, > > This addition seems distinct from the bindings::* removal. Should it > have been a separate patch ? SysBusDevice is re-exported from sysbus, similar to DeviceState or VMStateDescription that you see in the context. This is done because in general using "bindings" is an anti-pattern; you should use APIs that have been wrapped properly as is the case for SysBusDevice. The tests use some "bindings::foo" symbols either because they're setting up the test environment (module_call_init) or because they're intentionally bypassing the high-level API (object_new/unref). The only case where something isn't yet covered by safe wrappers is qdev_prop_bool. In fact, a lot of the structs that the tests need were imported twice, once from bindings and once from e.g. qdev or vmstate. SysBusDevice was the odd one out that was imported only from bindings, so it has added. Paolo > > vmstate::VMStateDescription, > > zeroable::Zeroable, > > }; > > With regards, > Daniel
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs index 92dbfb8a0c8..03569e4a44c 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -8,13 +8,14 @@ }; use qemu_api::{ - bindings::*, + bindings::{module_call_init, module_init_type, object_new, object_unref, qdev_prop_bool}, c_str, cell::{self, BqlCell}, declare_properties, define_property, prelude::*, qdev::{DeviceClass, DeviceImpl, DeviceState, Property, ResettablePhasesImpl}, qom::{ClassInitImpl, ObjectImpl, ParentField}, + sysbus::SysBusDevice, vmstate::VMStateDescription, zeroable::Zeroable, };
Similar to the devices, spell the exact set of C functions that are called directly. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- rust/qemu-api/tests/tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)