Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()

[Prasad Pandit]

Hello Zhao,

> (Communicating with you also helped me to understand the QAPI related parts.)

* I'm also visiting QAPI code parts for the first time. Thanks to you. :)

On Mon, 11 Mar 2024 at 10:36, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> SMPConfiguration is created and set in machine_set_smp().
> Firstly, it is created by g_autoptr(), and then it is initialized by
> visit_type_SMPConfiguration().
>
> The visit_type_SMPConfiguration() is generated by
> gen_visit_object_members() in scripts/qapi/visit.py.
>
> IIUC, in visit_type_SMPConfiguration() (let's have a look at
> gen_visit_object_members()), there's no explicit initialization of
> SMPConfiguration() (i.e., the parameter named "%(c_name)s *obj" in
> gen_visit_object_members()). For int type, has_* means that the field is
> set.
>

* Thank you for the above details, appreciate it. I added few
fprintf() calls to visit_type_SMPConfiguration() to see what user
values it receives
===
$ ./qemu-system-x86_64 -smp cores=2,maxcpus=2,cpus=1,sockets=2
visit_type_SMPConfiguration: name: smp
        has_cpus: 1:1
 has_drawvers: 0:0
      has_books: 0:0
  has_sockets: 1:2
        has_dies: 0:0
 has_clusters: 0:0
     has_cores: 1:2
  has_threads: 0:0
has_maxcpus: 1:2
qemu-system-x86_64: Invalid CPU topology: product of the hierarchy
must match maxcpus: sockets (2) * dies (1) * cores (2) * threads (0)
!= maxcpus (2)
===
* As seen above, undefined -smp fields (both has_xxxx and xxxx) are
set to zero(0).

===
main
 qemu_init
  qemu_apply_machine_options
   object_set_properties_from_keyval
    object_set_properties_from_qdict
     object_property_set
      machine_set_smp
       visit_type_SMPConfiguration
        visit_start_struct
(gdb) p v->start_struct
$4 = ... 0x5555570248e4 <qobject_input_start_struct>
(gdb)
(gdb)
 qobject_input_start_struct
   if (obj) {
        *obj = g_malloc0(size);
    }
===
* Stepping through function calls in gdb(1) revealed above call
sequence which leads to  'SMPConfiguration **'  objects allocation by
g_malloc0.

> This means if user doesn't initialize some field, the the value should
> be considerred as unreliable. And I guess for int, the default
> initialization value is the same as if we had declared a regular integer
> variable. But anyway, fields that are not explicitly initialized should
> not be accessed.

* g_malloc0() allocating 'SMPConfiguration **' object above assures us
that undefined -smp fields shall always be zero(0).

* 'obj->has_xxxx' field is set only if the user has supplied the
variable value, otherwise it remains set to zero(0).
   visit_type_SMPConfiguration_members
     ->visit_optional
       ->v->optional
        -> qobject_input_optional

* Overall, I think there is scope to simplify the
'machine_parse_smp_config()' function by using SMPConfiguration
field(s) ones.

Thank you.
---
  - Prasad