On Tue, Oct 20, 2015 at 09:57:49 -0400, John Ferlan wrote:
> On 10/16/2015 08:11 AM, Peter Krempa wrote:
> > Since we already make sure before that the domain configuration is
> > valid we may execute it always at the cost of doing 0 iterations of the
> > for loop.
> >
> > This patch will simplify later refactor as it will avoid whitespace
> > changes.
> > ---
> > src/qemu/qemu_command.c | 47 +++++++++++++++++++++++------------------------
> > 1 file changed, 23 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index a37a4fa..43e81c7 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -9484,38 +9484,37 @@ qemuBuildCommandLine(virConnectPtr conn,
> > }
> > }
> >
> > - if (virDomainNumaGetNodeCount(def->numa)) {
> > - if (qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0)
> > + if (virDomainNumaGetNodeCount(def->numa) &&
> > + qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0)
> > goto error;
> >
> > - /* memory hotplug requires NUMA to be enabled - we already checked
> > - * that memory devices are present only when NUMA is */
> > + /* memory hotplug requires NUMA to be enabled - we already checked
> > + * that memory devices are present only when NUMA is */
>
> The second half of this comment made me wonder where... especially with
> the removal of the virDomainNumaGetNodeCount call which would
> essentially be checking if numa was enabled (I think).
>
> The virDomainDefHasMemoryHotplug checks memory_slots or max_memory
>
> The virDomainDefPostParseMemory could have it, but it's not overt
>
> Not that the check is wrong, but it would be nice to fixup the comment
> to indicate that 'xxx' checked that memory devices are present only when
> NUMA is. That at least helps ensure any future refactor doesn't impact
> the assumption made here.
Since the next commit is actually moving the place where it's checked
and the later commits invalidate that commit since it will work even
without NUMA at all I will not bother modifying it here.
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list