qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Question] What is the definition of “private” fields in QOM?
@ 2024-10-19 16:10 Zhao Liu
  2024-10-21  8:35 ` Daniel P. Berrangé
  2024-10-21  9:25 ` Peter Maydell
  0 siblings, 2 replies; 15+ messages in thread
From: Zhao Liu @ 2024-10-19 16:10 UTC (permalink / raw)
  To: Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, qemu-devel; +Cc: Zhao Liu

Hi maintainers and list,

In the QOM structure, the class and object structs have two members:
parent_class and parent_obj, which are often marked as "< private >" in
the comment.

I couldn’t find information on why to define ‘private’ and ‘public’,
even in the earliest QOM commits and the patch emails I could find.

Does ‘private’ refer to the internal implementation code of QOM, or does
it refer to the specific code that defines and implements this object
and its class?

I understand the original idea of private field indicates it cannot be
accessed directly out of the "private" scope.

Regards,
Zhao



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-19 16:10 [Question] What is the definition of “private” fields in QOM? Zhao Liu
@ 2024-10-21  8:35 ` Daniel P. Berrangé
  2024-10-21  9:22   ` Zhao Liu
  2024-10-21  9:25 ` Peter Maydell
  1 sibling, 1 reply; 15+ messages in thread
From: Daniel P. Berrangé @ 2024-10-21  8:35 UTC (permalink / raw)
  To: Zhao Liu; +Cc: Paolo Bonzini, Eduardo Habkost, qemu-devel

On Sun, Oct 20, 2024 at 12:10:14AM +0800, Zhao Liu wrote:
> Hi maintainers and list,
> 
> In the QOM structure, the class and object structs have two members:
> parent_class and parent_obj, which are often marked as "< private >" in
> the comment.
> 
> I couldn’t find information on why to define ‘private’ and ‘public’,
> even in the earliest QOM commits and the patch emails I could find.
> 
> Does ‘private’ refer to the internal implementation code of QOM, or does
> it refer to the specific code that defines and implements this object
> and its class?
> 
> I understand the original idea of private field indicates it cannot be
> accessed directly out of the "private" scope.

I see two scenarios

 * Devices where the structs are in the include/..../<blah>.h

   The private/public comments are a message to other code in QEMU about
   which fields are OK to access directly, and which should not be accessed.


 * Devices where the structs are in the ../<blah>.c

   The private/public comments look entirely pointless, as everything
   is private to the .c file.


99% of the time it seems the "parent" / "parent_obj" fields are the only
one marked private. There are a handful of classes where other fields are
under the private comment, but not many. 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-21  8:35 ` Daniel P. Berrangé
@ 2024-10-21  9:22   ` Zhao Liu
  0 siblings, 0 replies; 15+ messages in thread
From: Zhao Liu @ 2024-10-21  9:22 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: Paolo Bonzini, Eduardo Habkost, qemu-devel

Hi Daniel,

On Mon, Oct 21, 2024 at 09:35:22AM +0100, Daniel P. Berrangé wrote:
> Date: Mon, 21 Oct 2024 09:35:22 +0100
> From: "Daniel P. Berrangé" <berrange@redhat.com>
> Subject: Re: [Question] What is the definition of “private” fields in
>  QOM?
> 
> On Sun, Oct 20, 2024 at 12:10:14AM +0800, Zhao Liu wrote:
> > Hi maintainers and list,
> > 
> > In the QOM structure, the class and object structs have two members:
> > parent_class and parent_obj, which are often marked as "< private >" in
> > the comment.
> > 
> > I couldn’t find information on why to define ‘private’ and ‘public’,
> > even in the earliest QOM commits and the patch emails I could find.
> > 
> > Does ‘private’ refer to the internal implementation code of QOM, or does
> > it refer to the specific code that defines and implements this object
> > and its class?
> > 
> > I understand the original idea of private field indicates it cannot be
> > accessed directly out of the "private" scope.
> 
> I see two scenarios
> 
>  * Devices where the structs are in the include/..../<blah>.h
> 
>    The private/public comments are a message to other code in QEMU about
>    which fields are OK to access directly, and which should not be accessed.

Does "other code" refer to the fact that no arbitrary code should
directly access parent_obj/parent_class, but rather should use an
explicit cast to convert to the parent class/object?

For example, are the following direct accesses to parent_obj all
considered illegal?

hw/usb/hcd-ohci-pci.c:58:    pci_set_word(dev->parent_obj.config + PCI_STATUS,
hw/arm/smmuv3.c:1094:        trace_smmuv3_translate_success(mr->parent_obj.name, sid, addr,
hw/arm/smmuv3.c:1101:        trace_smmuv3_translate_disable(mr->parent_obj.name, sid, addr,
hw/arm/smmuv3.c:1107:        trace_smmuv3_translate_bypass(mr->parent_obj.name, sid, addr,
hw/arm/smmuv3.c:1112:        trace_smmuv3_translate_abort(mr->parent_obj.name, sid, addr,
hw/arm/smmuv3.c:1119:                      mr->parent_obj.name, addr, smmu_event_string(event.type));
hw/arm/smmuv3.c:1212:        trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, vmid,
hw/arm/smmuv3.c:2033:        trace_smmuv3_notify_flag_add(iommu->parent_obj.name);
hw/arm/smmuv3.c:2036:        trace_smmuv3_notify_flag_del(iommu->parent_obj.name);
hw/arm/smmu-common.c:888:    trace_smmu_inv_notifiers_mr(mr->parent_obj.name);
block/throttle-groups.c:777:    if (!tg->name && tg->parent_obj.parent) {
iothread.c:173:                               iothread->parent_obj.aio_max_batch);
iothread.c:359:    info->aio_max_batch = iothread->parent_obj.aio_max_batch;
target/i386/sev.c:1240:    if (sev_snp->parent_obj.kernel_hashes) {
target/ppc/kvm.c:2354:    const char *vcpu_str = (cs->parent_obj.hotplugged == true) ?
target/openrisc/sys_helper.c:289:        return cpu->parent_obj.cpu_index;
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)) {
accel/tcg/translator.c:26:                    offsetof(ArchCPU, parent_obj.neg.can_do_io) -
accel/tcg/translator.c:50:                       offsetof(ArchCPU, parent_obj.neg.icount_decr.u32)
accel/tcg/translator.c:80:                         offsetof(ArchCPU, parent_obj.neg.icount_decr.u16.low)
include/hw/pci-host/dino.h:113:     * PCI_CONFIG_ADDR is parent_obj.config_reg, via pci_host_conf_be_ops,
...

>  * Devices where the structs are in the ../<blah>.c
> 
>    The private/public comments look entirely pointless, as everything
>    is private to the .c file.

Yes!

> 99% of the time it seems the "parent" / "parent_obj" fields are the only
> one marked private. There are a handful of classes where other fields are
> under the private comment, but not many. 

Yes. "parent" / "parent_obj" are the most common cases.

Thanks,
Zhao



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-19 16:10 [Question] What is the definition of “private” fields in QOM? Zhao Liu
  2024-10-21  8:35 ` Daniel P. Berrangé
@ 2024-10-21  9:25 ` Peter Maydell
  2024-10-21 14:22   ` Zhao Liu
  2024-10-21 18:46   ` BALATON Zoltan
  1 sibling, 2 replies; 15+ messages in thread
From: Peter Maydell @ 2024-10-21  9:25 UTC (permalink / raw)
  To: Zhao Liu; +Cc: Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, qemu-devel

On Sat, 19 Oct 2024 at 16:54, Zhao Liu <zhao1.liu@intel.com> wrote:
>
> Hi maintainers and list,
>
> In the QOM structure, the class and object structs have two members:
> parent_class and parent_obj, which are often marked as "< private >" in
> the comment.
>
> I couldn’t find information on why to define ‘private’ and ‘public’,
> even in the earliest QOM commits and the patch emails I could find.

This is a rather old thing which I think was originally
borrowed from glib's commenting convention.

I'm fairly sure that we decided a while back that they were entirely
unnecessary, so you don't need to add them in new code. (I can't
actually find anything with a quick list search about that though
so maybe I'm misremembering.)

Either way, there's still a lot of them floating around in the codebase
that were added before we made that decision.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-21 14:22   ` Zhao Liu
@ 2024-10-21 14:20     ` Peter Maydell
  2024-10-21 15:18       ` Zhao Liu
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2024-10-21 14:20 UTC (permalink / raw)
  To: Zhao Liu; +Cc: Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, qemu-devel

On Mon, 21 Oct 2024 at 15:12, Zhao Liu <zhao1.liu@intel.com> wrote:
>
> Hi Peter,
>
> On Mon, Oct 21, 2024 at 10:25:07AM +0100, Peter Maydell wrote:
> > Date: Mon, 21 Oct 2024 10:25:07 +0100
> > From: Peter Maydell <peter.maydell@linaro.org>
> > Subject: Re: [Question] What is the definition of “private” fields in
> >  QOM?
> >
> > On Sat, 19 Oct 2024 at 16:54, Zhao Liu <zhao1.liu@intel.com> wrote:
> > >
> > > Hi maintainers and list,
> > >
> > > In the QOM structure, the class and object structs have two members:
> > > parent_class and parent_obj, which are often marked as "< private >" in
> > > the comment.
> > >
> > > I couldn’t find information on why to define ‘private’ and ‘public’,
> > > even in the earliest QOM commits and the patch emails I could find.
> >
> > This is a rather old thing which I think was originally
> > borrowed from glib's commenting convention.
> >
> > I'm fairly sure that we decided a while back that they were entirely
> > unnecessary, so you don't need to add them in new code. (I can't
> > actually find anything with a quick list search about that though
> > so maybe I'm misremembering.)
>
> Thanks for your explanation! So I understand that directly accessing
> parent_obj/parent_class is actually allowed.

No, you shouldn't do that. You can use a QOM cast of the
object pointer to the relevant parent class if you need to
treat it as an instance of the parent class.

What I mean by "the private/public markers are unnecessary" is
that they don't tell the reader anything, because all the fields
in a QOM device struct are private.

If you're not in the implementation of that class, then you shouldn't
really be directly touching any of the fields in the state struct.
(In some places we take a shortcut and do it. But really it's almost
never necessary.)

thanks
-- PMM


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-21  9:25 ` Peter Maydell
@ 2024-10-21 14:22   ` Zhao Liu
  2024-10-21 14:20     ` Peter Maydell
  2024-10-21 18:46   ` BALATON Zoltan
  1 sibling, 1 reply; 15+ messages in thread
From: Zhao Liu @ 2024-10-21 14:22 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, qemu-devel

Hi Peter,

On Mon, Oct 21, 2024 at 10:25:07AM +0100, Peter Maydell wrote:
> Date: Mon, 21 Oct 2024 10:25:07 +0100
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: Re: [Question] What is the definition of “private” fields in
>  QOM?
> 
> On Sat, 19 Oct 2024 at 16:54, Zhao Liu <zhao1.liu@intel.com> wrote:
> >
> > Hi maintainers and list,
> >
> > In the QOM structure, the class and object structs have two members:
> > parent_class and parent_obj, which are often marked as "< private >" in
> > the comment.
> >
> > I couldn’t find information on why to define ‘private’ and ‘public’,
> > even in the earliest QOM commits and the patch emails I could find.
> 
> This is a rather old thing which I think was originally
> borrowed from glib's commenting convention.
> 
> I'm fairly sure that we decided a while back that they were entirely
> unnecessary, so you don't need to add them in new code. (I can't
> actually find anything with a quick list search about that though
> so maybe I'm misremembering.)

Thanks for your explanation! So I understand that directly accessing
parent_obj/parent_class is actually allowed.

> Either way, there's still a lot of them floating around in the codebase
> that were added before we made that decision.

Yes, then I understand that <private> and <public> are historical
burdens that can also be cleaned up.

Thanks,
Zhao



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-21 15:18       ` Zhao Liu
@ 2024-10-21 15:06         ` Peter Maydell
  2024-10-21 15:41           ` Zhao Liu
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2024-10-21 15:06 UTC (permalink / raw)
  To: Zhao Liu; +Cc: Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, qemu-devel

On Mon, 21 Oct 2024 at 16:02, Zhao Liu <zhao1.liu@intel.com> wrote:
>
> On Mon, Oct 21, 2024 at 03:20:39PM +0100, Peter Maydell wrote:
> > What I mean by "the private/public markers are unnecessary" is
> > that they don't tell the reader anything, because all the fields
> > in a QOM device struct are private.
>
> This time I really understand the question of whether it's okay to
> directly access parent_obj/parent_class. :-)
>
> > If you're not in the implementation of that class, then you shouldn't
> > really be directly touching any of the fields in the state struct.
> > (In some places we take a shortcut and do it. But really it's almost
> > never necessary.)
>
> Thank you for your further explanation! I hadn’t noticed that. So, for
> other code (code outside the class/object implementation) to access the
> fields other than parent_obj/parent_class of class/state struct, the
> most ideal way would be to use the set/get property interfaces as
> much as possible instead of accessing them directly, right?

Yes, or whatever APIs (functions etc) are provided for working
with the class. If you have a specific example we could probably
make some more concrete suggestions.

-- PMM


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-21 14:20     ` Peter Maydell
@ 2024-10-21 15:18       ` Zhao Liu
  2024-10-21 15:06         ` Peter Maydell
  0 siblings, 1 reply; 15+ messages in thread
From: Zhao Liu @ 2024-10-21 15:18 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, qemu-devel

On Mon, Oct 21, 2024 at 03:20:39PM +0100, Peter Maydell wrote:
> Date: Mon, 21 Oct 2024 15:20:39 +0100
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: Re: [Question] What is the definition of “private” fields in
>  QOM?
> 
> On Mon, 21 Oct 2024 at 15:12, Zhao Liu <zhao1.liu@intel.com> wrote:
> >
> > Hi Peter,
> >
> > On Mon, Oct 21, 2024 at 10:25:07AM +0100, Peter Maydell wrote:
> > > Date: Mon, 21 Oct 2024 10:25:07 +0100
> > > From: Peter Maydell <peter.maydell@linaro.org>
> > > Subject: Re: [Question] What is the definition of “private” fields in
> > >  QOM?
> > >
> > > On Sat, 19 Oct 2024 at 16:54, Zhao Liu <zhao1.liu@intel.com> wrote:
> > > >
> > > > Hi maintainers and list,
> > > >
> > > > In the QOM structure, the class and object structs have two members:
> > > > parent_class and parent_obj, which are often marked as "< private >" in
> > > > the comment.
> > > >
> > > > I couldn’t find information on why to define ‘private’ and ‘public’,
> > > > even in the earliest QOM commits and the patch emails I could find.
> > >
> > > This is a rather old thing which I think was originally
> > > borrowed from glib's commenting convention.
> > >
> > > I'm fairly sure that we decided a while back that they were entirely
> > > unnecessary, so you don't need to add them in new code. (I can't
> > > actually find anything with a quick list search about that though
> > > so maybe I'm misremembering.)
> >
> > Thanks for your explanation! So I understand that directly accessing
> > parent_obj/parent_class is actually allowed.
> 
> No, you shouldn't do that. You can use a QOM cast of the
> object pointer to the relevant parent class if you need to
> treat it as an instance of the parent class.
>
> What I mean by "the private/public markers are unnecessary" is
> that they don't tell the reader anything, because all the fields
> in a QOM device struct are private.

This time I really understand the question of whether it's okay to
directly access parent_obj/parent_class. :-)

> If you're not in the implementation of that class, then you shouldn't
> really be directly touching any of the fields in the state struct.
> (In some places we take a shortcut and do it. But really it's almost
> never necessary.)

Thank you for your further explanation! I hadn’t noticed that. So, for
other code (code outside the class/object implementation) to access the
fields other than parent_obj/parent_class of class/state struct, the
most ideal way would be to use the set/get property interfaces as
much as possible instead of accessing them directly, right?

Regards,
Zhao



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-21 15:06         ` Peter Maydell
@ 2024-10-21 15:41           ` Zhao Liu
  2024-10-21 16:47             ` Peter Maydell
  0 siblings, 1 reply; 15+ messages in thread
From: Zhao Liu @ 2024-10-21 15:41 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, qemu-devel

On Mon, Oct 21, 2024 at 04:06:08PM +0100, Peter Maydell wrote:
> Date: Mon, 21 Oct 2024 16:06:08 +0100
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: Re: [Question] What is the definition of “private” fields in
>  QOM?
> 
> On Mon, 21 Oct 2024 at 16:02, Zhao Liu <zhao1.liu@intel.com> wrote:
> >
> > On Mon, Oct 21, 2024 at 03:20:39PM +0100, Peter Maydell wrote:
> > > What I mean by "the private/public markers are unnecessary" is
> > > that they don't tell the reader anything, because all the fields
> > > in a QOM device struct are private.
> >
> > This time I really understand the question of whether it's okay to
> > directly access parent_obj/parent_class. :-)
> >
> > > If you're not in the implementation of that class, then you shouldn't
> > > really be directly touching any of the fields in the state struct.
> > > (In some places we take a shortcut and do it. But really it's almost
> > > never necessary.)
> >
> > Thank you for your further explanation! I hadn’t noticed that. So, for
> > other code (code outside the class/object implementation) to access the
> > fields other than parent_obj/parent_class of class/state struct, the
> > most ideal way would be to use the set/get property interfaces as
> > much as possible instead of accessing them directly, right?
> 
> Yes, or whatever APIs (functions etc) are provided for working
> with the class. If you have a specific example we could probably
> make some more concrete suggestions.

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. 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…

Thanks,
Zhao



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-21 15:41           ` Zhao Liu
@ 2024-10-21 16:47             ` Peter Maydell
  2024-10-22  3:08               ` Junjie Mao
  2024-10-22 15:01               ` Zhao Liu
  0 siblings, 2 replies; 15+ messages in thread
From: Peter Maydell @ 2024-10-21 16:47 UTC (permalink / raw)
  To: Zhao Liu; +Cc: Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, qemu-devel

On Mon, 21 Oct 2024 at 16:25, Zhao Liu <zhao1.liu@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?

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-21  9:25 ` Peter Maydell
  2024-10-21 14:22   ` Zhao Liu
@ 2024-10-21 18:46   ` BALATON Zoltan
  1 sibling, 0 replies; 15+ messages in thread
From: BALATON Zoltan @ 2024-10-21 18:46 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Zhao Liu, Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost,
	qemu-devel

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

On Mon, 21 Oct 2024, Peter Maydell wrote:
> On Sat, 19 Oct 2024 at 16:54, Zhao Liu <zhao1.liu@intel.com> wrote:
>>
>> Hi maintainers and list,
>>
>> In the QOM structure, the class and object structs have two members:
>> parent_class and parent_obj, which are often marked as "< private >" in
>> the comment.
>>
>> I couldn’t find information on why to define ‘private’ and ‘public’,
>> even in the earliest QOM commits and the patch emails I could find.
>
> This is a rather old thing which I think was originally
> borrowed from glib's commenting convention.
>
> I'm fairly sure that we decided a while back that they were entirely
> unnecessary, so you don't need to add them in new code. (I can't
> actually find anything with a quick list search about that though
> so maybe I'm misremembering.)

I think the current convention was discussed around here:
https://lore.kernel.org/qemu-devel/33641fa3-f617-3151-e7ca-becaf06e2641@ilande.co.uk/
but looks like it did not make it into docs/devel/qom.rst to make it 
clear for all.

If your search found nothing on lists.gnu.org/archive maybe it has 
something to do with this message printed there:

References: [ (The index is locked for maintenance) ]

I don't know what that means but all searches seem to return the same 
currently.

Regards,
BALATON Zoltan

> Either way, there's still a lot of them floating around in the codebase
> that were added before we made that decision.
>
> thanks
> -- PMM
>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-21 16:47             ` Peter Maydell
@ 2024-10-22  3:08               ` Junjie Mao
  2024-10-22  8:42                 ` Peter Maydell
  2024-10-22 15:01               ` Zhao Liu
  1 sibling, 1 reply; 15+ messages in thread
From: Junjie Mao @ 2024-10-22  3:08 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Zhao Liu, Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost,
	qemu-devel


Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 21 Oct 2024 at 16:25, Zhao Liu <zhao1.liu@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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-22  3:08               ` Junjie Mao
@ 2024-10-22  8:42                 ` Peter Maydell
  2024-10-22 15:09                   ` Zhao Liu
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2024-10-22  8:42 UTC (permalink / raw)
  To: Junjie Mao
  Cc: Zhao Liu, Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost,
	qemu-devel

On Tue, 22 Oct 2024 at 04:24, Junjie Mao <junjie.mao@hotmail.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> > 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!.

Yes, conceptually I agree that the fields underlying a
property are private and the public interface is the
prop get/set API. (In C the prop/get set can if it
likes do things like enforcing value limits, so looking
directly at the underlying field would bypass that.)

At any rate it sounds like it would be a good idea to
at least mark as not-public all the fields we can do that
way, and have a comment
  /* pub only because they are properties */
for the fields used by the Property structs, even if we
don't yet have a better way to deal with the latter.

-- PMM


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-21 16:47             ` Peter Maydell
  2024-10-22  3:08               ` Junjie Mao
@ 2024-10-22 15:01               ` Zhao Liu
  1 sibling, 0 replies; 15+ messages in thread
From: Zhao Liu @ 2024-10-22 15:01 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, qemu-devel,
	Junjie Mao

On Mon, Oct 21, 2024 at 05:47:05PM +0100, Peter Maydell wrote:
> Date: Mon, 21 Oct 2024 17:47:05 +0100
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: Re: [Question] What is the definition of “private” fields in
>  QOM?
> 
> On Mon, 21 Oct 2024 at 16:25, Zhao Liu <zhao1.liu@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).

Thank you for taking me through this history, I see the intent of
designing the embedded device structure in this way!

> 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.

Very cool! May I ask a few more questions? :-) The feedback on this
series looks very positive, and it seems you were almost close to merge
it at the time. What ultimately led you to decide against it? If we
revisit the issue of Rust's private/pub visibility, I'm curious if we
would face the same dilemma again.

Regards,
Zhao



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Question] What is the definition of “private” fields in QOM?
  2024-10-22  8:42                 ` Peter Maydell
@ 2024-10-22 15:09                   ` Zhao Liu
  0 siblings, 0 replies; 15+ messages in thread
From: Zhao Liu @ 2024-10-22 15:09 UTC (permalink / raw)
  To: Peter Maydell, Junjie Mao
  Cc: Paolo Bonzini, Daniel P. Berrange, Eduardo Habkost, qemu-devel

On Tue, Oct 22, 2024 at 09:42:03AM +0100, Peter Maydell wrote:
> Date: Tue, 22 Oct 2024 09:42:03 +0100
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: Re: [Question] What is the definition of “private” fields in
>  QOM?
> 
> On Tue, 22 Oct 2024 at 04:24, Junjie Mao <junjie.mao@hotmail.com> wrote:
> > Peter Maydell <peter.maydell@linaro.org> writes:
> > > 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!.
> 
> Yes, conceptually I agree that the fields underlying a
> property are private and the public interface is the
> prop get/set API. (In C the prop/get set can if it
> likes do things like enforcing value limits, so looking
> directly at the underlying field would bypass that.)
> 
> At any rate it sounds like it would be a good idea to
> at least mark as not-public all the fields we can do that
> way, and have a comment
>   /* pub only because they are properties */
> for the fields used by the Property structs, even if we
> don't yet have a better way to deal with the latter.

Thank you both, Peter and Junjie!! I understand that the benefit of
declaring private states/classes in Rust is to avoid unnecessary
dependencies between different module/crates and to better manage
interactions between them. I'll go ahead and try out the methods you
both mentioned to compare them.

Regards,
Zhao



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2024-10-22 14:53 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-19 16:10 [Question] What is the definition of “private” fields in QOM? Zhao Liu
2024-10-21  8:35 ` Daniel P. Berrangé
2024-10-21  9:22   ` Zhao Liu
2024-10-21  9:25 ` Peter Maydell
2024-10-21 14:22   ` Zhao Liu
2024-10-21 14:20     ` Peter Maydell
2024-10-21 15:18       ` Zhao Liu
2024-10-21 15:06         ` Peter Maydell
2024-10-21 15:41           ` Zhao Liu
2024-10-21 16:47             ` Peter Maydell
2024-10-22  3:08               ` Junjie Mao
2024-10-22  8:42                 ` Peter Maydell
2024-10-22 15:09                   ` Zhao Liu
2024-10-22 15:01               ` Zhao Liu
2024-10-21 18:46   ` BALATON Zoltan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).