> A few code style comments inline.
>
> J
> > diff --git a/hw/cpu/cpu-slot.c b/hw/cpu/cpu-slot.c
> > index 1cc3b32ed675..2d16a2729501 100644
> > --- a/hw/cpu/cpu-slot.c
> > +++ b/hw/cpu/cpu-slot.c
>
> > +
> > +bool machine_parse_custom_topo_config(MachineState *ms,
> > + const SMPConfiguration *config,
> > + Error **errp)
> > +{
> > + MachineClass *mc = MACHINE_GET_CLASS(ms);
> > + CPUSlot *slot = ms->topo;
> > + bool is_valid;
> > + int maxcpus;
> > +
> > + if (!slot) {
> > + return true;
> > + }
> > +
> > + is_valid = config->has_maxsockets && config->maxsockets;
> > + if (mc->smp_props.custom_topo_supported) {
> > + slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit =
> > + is_valid ? config->maxsockets : ms->smp.sockets;
> > + } else if (is_valid) {
> > + error_setg(errp, "maxsockets > 0 not supported "
> > + "by this machine's CPU topology");
> > + return false;
> > + } else {
> > + slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit =
> > + ms->smp.sockets;
> > + }
> Having the error condition in the middle is rather confusing to
> read to my eyes. Playing with equivalents I wonder what works best..
Figuring out how to clearly express the logic here was indeed a bit of a
struggle for me at first. :-)
> if (!is_valid) {
> slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit =
> ms->smp.sockets;
> } else if (mc->smp_props.custom_topo_supported) {
> slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit =
> config->max_sockets;
> } else {
> error_setg...
> return false;
> }
>
> or take the bad case out first. Maybe this is a little obscure
> though (assuming I even got it right) as it relies on the fact
> that is_valid must be false for the legacy path.
>
> if (!mc->smp_props.custom_topo_supported && is_valid) {
> error_setg();
> return false;
> }
>
> slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit =
> is_valid ? config->maxsockets : ms->smp.sockets;
>
> Similar for other cases.
I prefer the first style, as it's more natural and clear enough!
Many thanks!
[snip]
> > + maxcpus = 1;
> > + /* Initizlize max_limit to 1, as members of CpuTopology. */
> > + for (int i = 0; i < CPU_TOPOLOGY_LEVEL__MAX; i++) {
> > + maxcpus *= slot->stat.entries[i].max_limit;
> > + }
> > +
> > + if (!config->has_maxcpus) {
> > + ms->smp.max_cpus = maxcpus;
> Maybe early return here to get rid of need for the else?
Yes, it's better to reduce else.
> > + } else {
> > + if (maxcpus != ms->smp.max_cpus) {
>
> Unless this is going to get more complex later, else if probably appropriate here
> (if you don't drop the else above.
I can organize it like:
if (!config->has_maxcpus) {
...
return true;
}
if (maxcpus != ms->smp.max_cpus) {
error_steg...
return false;
}
return true;
As you suggested to get rid of a "else". :)
> > + error_setg(errp, "maxcpus (%d) should be equal to "
> > + "the product of the remaining max parameters (%d)",
> > + ms->smp.max_cpus, maxcpus);
> > + return false;
> > + }
> > + }
> > +
> > + return true;
> > +}
Regards,
Zhao
