On Wed, Jul 24, 2019 at 05:48:11PM +0200, Igor Mammedov wrote: > On Wed, 24 Jul 2019 12:02:41 -0300 > Eduardo Habkost <ehabk...@redhat.com> 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 <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, > 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