hw: Fix abuse of QOM class internals modified by their instances

[PATCH 0/3] hw: Fix abuse of QOM class internals modified by their instances

Philippe Mathieu-Daudé posted 3 patches 11 months, 2 weeks ago

Download series mbox

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230523064408.57941-1-philmd@linaro.org

Maintainers: Titus Rwantare <titusr@google.com>, "Hervé Poussineau" <hpoussin@reactos.org>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>

hw/i2c/pmbus_device.c | 17 ++++++++++-------
hw/mips/jazz.c        | 41 +++++++++++++++++++++++------------------
hw/ppc/e500plat.c     | 25 +++++++++++--------------
3 files changed, 44 insertions(+), 39 deletions(-)

Expand all Fold all

[PATCH 0/3] hw: Fix abuse of QOM class internals modified by their instances

Posted by Philippe Mathieu-Daudé 11 months, 2 weeks ago

Bernhard warned for QOM class abuse here [*]:

> A realize method is supposed to modify a single instance only
> while we're modifying the behavior of whole classes here, i.e.
> will affect every instance of these classes. This goes against
> QOM design principles and will therefore be confusing for people
> who are familiar with QOM in particular and OOP in general.

This series fixes the cases I found while auditing.

[*] https://lore.kernel.org/qemu-devel/0DAAC63B-0C0F-44C4-B7EB-ACD6C9A36BF1@gmail.com/

Philippe Mathieu-Daudé (3):
  hw/mips/jazz: Fix modifying QOM class internal state from instance
  hw/ppc/e500plat: Fix modifying QOM class internal state from instance
  hw/i2c/pmbus_device: Fix modifying QOM class internals from instance

 hw/i2c/pmbus_device.c | 17 ++++++++++-------
 hw/mips/jazz.c        | 41 +++++++++++++++++++++++------------------
 hw/ppc/e500plat.c     | 25 +++++++++++--------------
 3 files changed, 44 insertions(+), 39 deletions(-)

-- 
2.38.1

Re: [PATCH 0/3] hw: Fix abuse of QOM class internals modified by their instances

Posted by Philippe Mathieu-Daudé 11 months, 2 weeks ago

(posted too quickly)

On 23/5/23 08:44, Philippe Mathieu-Daudé wrote:
> Bernhard warned for QOM class abuse here [*]:
> 
>> A realize method is supposed to modify a single instance only
>> while we're modifying the behavior of whole classes here, i.e.
>> will affect every instance of these classes. This goes against
>> QOM design principles and will therefore be confusing for people
>> who are familiar with QOM in particular and OOP in general.
> 
> This series fixes the cases I found while auditing.

Audited but not yet fixed:

- accel/xen
   xen_init() sets 'default_ram_id'
   -> need to figure migration compatibility (because using shared
      pc_machine_class_init and DEFINE_PC_MACHINE), will be posted
      separately.

- hw/core/bus
   qbus_init_internal() increments 'automatic_ids'
   -> need more thought.

- hw/core/
   machine_parse_smp_config() sets 'smp_props.has_clusters'
   -> dubious

- hw/remote/
   remote_object_init() and probe_pci_info() set 'vendor_id', 'nr_devs'
   -> need more thought.

- qom/object/
   object_dynamic_cast_assert() populates object_cast_cache[]
   -> QOM internal, likely OK.

- softmmu/vl
   configure_blockdev() overwrites 'machine_class->block_default_type'
   -> dubious

- gdbstub
   ppc_gdb_gen_spr_xml() sets 'gdb_num_sprs' and 'gdb_spr_xml'
   -> dubious, likely fixable (maybe like patch #3 of this series)

- tests/unit/
   smp_parse_test() set 'smp_props.prefer_sockets'
   -> dubious, probably tolerable.

> [*] https://lore.kernel.org/qemu-devel/0DAAC63B-0C0F-44C4-B7EB-ACD6C9A36BF1@gmail.com/