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

[Zhao Liu <zhao1.liu@linux.intel.com>]

Hi Prasad (and also welcome folks correct me),

> On Fri, 8 Mar 2024 at 20:50, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> > On Fri, Mar 08, 2024 at 02:20:45PM +0100, Thomas Huth wrote:
> > > Can we always rely on that? ... or is this just by luck due to the current
> > > implementation? In the latter case, I'd maybe rather drop this patch again.
> >
> > Thanks for the correction, I revisited and referenced more similar use
> > cases, and indeed, only if the flag "has_*" is true, its corresponding
> > field should be considered reliable.
> 
> * Is this because 'SMPConfiguration config'  fields are not always
> initialized with default values?

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.

The g_autoptr() requires user to initialize:

  You must initialise the variable in some way — either by use of an
  initialiser or by ensuring that it is assigned to unconditionally
  before it goes out of scope.

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.

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.

Therefore, we shouldn't rely on the uninitialized fields and should
check the has_*.

> Is that a bug?

It's not a bug, and it could be a simplification of the automatic code
generation.

> Having 'SMPConfiguration' fields initialised to known default values could
> help to unify/simplify code which uses those fields.

I realized that keeping the check for has_* here would help improve code
readability; after all, it's more of a pain to go back and check
scripts.

> > Keeping explicit checking on has_* and explicit initialization of these
> > topology variables makes the code more readable.
> >
> > This patch is over-optimized and I would drop it.
> 
> * Could we then simplify it in the following if <expression>
> ===
>       if ((config->has_cpus && config->cpus == 0) ||
>           ... ||
>           (config->has_maxcpus && config->maxcpus == 0))
> 
> could be
> 
>       if (!cpus || !drawers || ... || !maxcpus) { ... }
> ===
>

This doesn't work since the above check is used to identify if user sets
parameters as 0 explicitly, which needs to check both has_* and the
specific field value.

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

Thanks,
Zhao