Peter Maydell <peter.mayd...@linaro.org> writes:
> On Mon, 21 Oct 2024 at 16:25, Zhao Liu <zhao1....@intel.com> wrote:
>> My initial confusion stemmed from seeing the private comment and
>> noticing that there are many direct accesses to parent_obj/parent_class
>> in QEMU (which I could list in my reply to Daniel). Now I understand
>> that these examples are only valid within the class/object
>> implementation or in QOM code.
>>
>> For instance, an example in KVM is a misuse:
>>
>> accel/kvm/kvm-all.c:4285:
>> cpu->parent_obj.canonical_path,
>> accel/kvm/kvm-all.c:4377: if
>> (!apply_str_list_filter(cpu->parent_obj.canonical_path, targets)) {
>>
>>
>> At the same time, I’ve been thinking that the current C implementation
>> seems to have no way to prevent such misuse.
>
> Mmm. We rely on code review to catch major misuses (and let
> some minor misuses slide because we don't care enough to put
> in the effort to provide a proper API to access the information
> or because we don't want the performance overhead of a QOM cast).
>
> In a previous iteration of QEMU's design the device's state
> struct was purely private to the implementation source file,
> and code that used the device always did so via a pointer.
> But at some point we decided we wanted to allow users to
> embed the device struct inside their own device struct, which
> needs them to have access to the struct definition (though
> strictly they only need to know the size and alignment
> requirements of it).
>
> I did a decade ago write a proof-of-concept for marking
> fields in the C struct as "private" such that you could get
> a compile error for touching them:
> https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg01846.html
> which (mis?)uses GCC's __attribute__((deprecated("reason")))
> to arrange that touching the struct field from outside the
> implementation is a compile error. But we never took up the idea.
>
>> So for Rust's class/state,
>> should parent_class/parent_obj also be defined as private (for example,
>> by removing the pub keyword from PL011State in rust/hw/char/pl011/src/
>> device.rs)?
>>
>> However, through our discussion, I realized that in QOM, "private" does
>> not only refer to parent_obj/parent_class, but all fields belong to
>> this category. If everything is strictly defined as private in Rust, it
>> seems impractical…
>
> For Rust we get to make a fresh start on these things. If
> we do mark all these fields not public, what would break?
>
The only thing that breaks today is std::mem::offset_of! which respects
field visibility. Defining a Property const structure requires getting
the field offset outside of the state context.
To me properties are still private to the device state and must be
accessed via their getters & setters. A solution to that is to keep
properties private but make their offsets public in our alternative to
offset_of!.
--
Best Regards
Junjie Mao
> I do think that some of these fields really are internal
> implementation details -- only the PL011 UART itself
> should be directly accessing PL011State::read_fifo, for example.
>
> thanks
> -- PMM
