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

[Zhao Liu]

On Wed, Mar 13, 2024 at 04:22:30PM +0530, Prasad Pandit wrote:
> Date: Wed, 13 Mar 2024 16:22:30 +0530
> From: Prasad Pandit <ppandit@redhat.com>
> Subject: Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables'
>  initialization in machine_parse_smp_config()
> 
> 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.

Thanks! I misunderstood, it turns out that the initialization is done here.

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

Yes, you're right!

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

Indeed, as you say, these items are initialized to 0 by default.

I think, however, that the initialization is so far away from where the
smp is currently parsed that one can't easily confirm it (thanks for
your confirmation!).

>From a code readability view, the fact that we're explicitly
initializing to 0 again here brings little overhead, but makes the code
more readable as well as easier to maintain. I think the small redundancy
here is worth it.

Also, in other use cases people always relies on fields marked by has_*,
and there is no (or less?) precedent for direct access to places where
has_* is not set. I understand that this is also a habit, i.e., fields
with a has_* of False by default are unreliable and avoid going directly
to them.

Regards,
Zhao