On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
> On Tue, 23 Jul 2019 12:23:57 -0300
> Eduardo Habkost <ehabk...@redhat.com> 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 <tao3...@intel.com> 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 <imamm...@redhat.com>
> > > > Suggested-by: Eduardo Habkost <ehabk...@redhat.com>
> > > > Signed-off-by: Tao Xu <tao3...@intel.com>
> > > > ---
> > > >
> > > > 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, and leads to hard to spot bugs like
the one you saw above.
Although I won't reject a patch just because it doesn't have a
machine_num_numa_nodes() wrapper, I insist we use one for clarity
and safety.
--
Eduardo
