On Fri, Jul 26, 2019 at 03:43:43PM +0200, Igor Mammedov wrote:
On Wed, 24 Jul 2019 15:15:28 -0300
Eduardo Habkost <address@hidden> wrote:
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?
read the code/patch
(at least I don't review just by looking at one line. And less time
I have to spend, on reading extra code and finding answers why it's
written the way it's, the better)
In this patch code touching ms->numa_state, is divided in 2 categories
generic code (memory API, CLI entry point, generic machine call
site for numa specific code, devices, monitor/qmp) and numa aware code
(huma parser and numa aware machines). The later one is majority of
affected code where ms->numa_state != NULL.
Even after I forget how this works and read code later, it would be
easy to do educated guess/check where NULL check is not need seeing
related code.
It's even easier to not have to check if/when numa_state can be
NULL because the code is safe on either case.
You don't review code by looking at a single line, but you don't
need to make it harder than it is.
With machine_num_numa_nodes() would have to look for answer why we
are doing it (unless we add a comment that check is there for noreason
in most cases and it's exercise for reader to find out where
it it's really need).
Sorry, your justification doesn't make sense to me. You don't
need to look for any answer at all, if the code makes it not
matter if numa_state is NULL. Having a single caller with
numa_state==NULL would be enough justification for the check.
I don't see any justification for wrapper this case,
could we stop bikeshedding and just let author to move on with fixing bugs, pls?
The author can move on and decide what to do, as I won't block a
patch only because of presence or absence of the wrapper.