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

[Eduardo Habkost]

On Wed, Jul 24, 2019 at 05:48:11PM +0200, Igor Mammedov wrote:
> On Wed, 24 Jul 2019 12:02:41 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
> > > 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.
> > 
> > I strongly disagree, here.  Avoiding a ms->numa_state==NULL check
> > is pointless optimization,
> I see it not as optimization (compiler probably would manage to optimize out 
> most of them)
> but as rather properly self documented code. Doing check in places where it's
> not needed is confusing at best and can mask/introduce later subtle bugs at 
> worst.
> 
> > and leads to hard to spot bugs like
> > the one you saw above.
> That one was actually easy to spot because of the way it's written in this 
> patch.

When somebody is looking at a line of code containing
"ms->numa_state->num_nodes", how exactly are they supposed to
know if ms->numa_state is already guaranteed to be non-NULL, or
not?

-- 
Eduardo