Paolo Bonzini <[email protected]> writes:
> On 2/10/25 10:59, Zhao Liu wrote: >> On Thu, Feb 06, 2025 at 12:15:14PM +0100, Paolo Bonzini wrote: >>> Not a major change but, as a small but significant step in creating >>> qdev bindings, show how pl011_create can be written without "unsafe" >>> calls (apart from converting pointers to references). >>> >>> This also provides a starting point for creating Error** bindings. >>> >>> Signed-off-by: Paolo Bonzini <[email protected]> >>> --- >>> rust/hw/char/pl011/src/device.rs | 39 ++++++++++++++++---------------- >>> rust/qemu-api/src/sysbus.rs | 34 +++++++++++++++++++++++++--- >>> 2 files changed, 50 insertions(+), 23 deletions(-) >> ... >> >>> + fn realize(&self) { >> What about renaming this as "realize_with_sysbus"? >> Elsewhere, the device's own realize method is often used to set >> DeviceImpl::REALIZE. > > I *think* this is not a problem in practice because trait methods are public > (if > the trait is in scope) whereas the device's realize method if private. I would suggest to keep the "sysbus" prefix in the method name, or in general, keep the class prefix in the method names in XXXClassMethods traits. Otherwise APIs from different parent classes may also conflict. As an example, in the following class hierarchy: Object -> DeviceState -> PCIDevice -> PCIBridge -> ... For PCIDevice instances there is an API pci_device_reset(), and for PCIBridge ones pci_bridge_reset(). Without class prefixes both will be wrapped (as blanket impl in two traits) as reset() and a dev.reset() call will lead to a "multiple applicable items in scope" build error. In addition, it is quite common to see static functions named xxx_device_type_reset() in C today. Thus, it is quite natural to expect (private) methods named XXXDeviceState::reset() in future Rust devices, which will cause even more trouble as the compiler will no longer complain and always pick that method for dev.reset() calls. More general names can be found in multiple device classes, such as write_config (pci_default_write_config() and pci_bridge_write_config()). There are alternatives to solve such conflicts, such as requiring proc macro attributes above methods with such general names, and requiring ambiguous parent class API call to use fully qualified syntax, e.g., SysBusDeviceMethods::realize(self). But I think the former can be error-prone because the list of general names vary among different device types and required attributes can be easily neglected since no build error is triggered without them, and the later is more tedious than self.sysbus_realize(). > > I agree that the naming conflict is unfortunate though, if only because it can > cause confusion. I don't know if this can be solved by procedural macros; for > example a #[vtable] attribute that changes > > #[qemu_api_macros::vtable] > fn realize(...) { > } > > into > > const fn REALIZE: ... = Some({ > fn realize(...) { > } > realize > } > > This way, the REALIZE item would be included in the "impl DeviceImpl for > PL011State" block instead of "impl PL011State". It's always a fine line > between procedural macros cleaning vs. messing things up, which is why until > now > I wanted to see what things look like with pure Rust code; but I guess now > it's > the time to evaluate this kind of thing. > > Adding Junjie since he contributed the initial proc macro infrastructure and > may > have opinions on this. I agree that uses of proc macros should be carefully evaluated to see if they really help or hurt. In this specific case, I'm not sure if using attributes solves the root cause, though. -- Best Regards Junjie Mao > > Paolo
