Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState

[Igor Mammedov]

On Tue, 23 Jul 2019 12:23:57 -0300
Eduardo Habkost <address@hidden> wrote:

> On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
> > On Tue, 16 Jul 2019 22:51:12 +0800
> > Tao Xu <address@hidden> wrote:
> > 
> > > Add struct NumaState in MachineState and move existing numa global
> > > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
> > > numa_support into MachineClass to decide which submachines support NUMA.
> > > 
> > > Suggested-by: Igor Mammedov <address@hidden>
> > > Suggested-by: Eduardo Habkost <address@hidden>
> > > Signed-off-by: Tao Xu <address@hidden>
> > > ---
> > > 
> > > No changes in v7.
> > > 
> > > Changes in v6:
> > >     - Rebase to upstream, move globals in arm/sbsa-ref and use
> > >       numa_mem_supported
> > >     - When used once or twice in the function, use
> > >       ms->numa_state->num_nodes directly
> > >     - Correct some mistakes
> > >     - Use once monitor_printf in hmp_info_numa
> > > ---
> [...]
> > >      if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
> > > -        pxb->numa_node >= nb_numa_nodes) {
> > > +        pxb->numa_node >= ms->numa_state->num_nodes) {
> > this will crash if user tries to use device on machine that doesn't support 
> > numa
> > check that numa_state is not NULL before dereferencing 
> 
> That's exactly why the machine_num_numa_nodes() was created in
> v5, but then you asked for its removal.
V4 to more precise.
I dislike small wrappers because they usually doesn't simplify code and make it 
more obscure,
forcing to jump around to see what's really going on.
Like it's implemented in this patch it's obvious what's wrong right away.

In that particular case machine_num_numa_nodes() was also misused since only a 
handful
of places (6) really need NULL check while majority (48) can directly access 
ms->numa_state->num_nodes.
without NULL check.