[libvirt] [PATCH v3 12/13] Implement CPU selection in QEMU driver

[Jiri Denemark]

> > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > +
> > +int
> > +qemudProbeCPUModels(const char *qemu,
> > +                    const char *arch,
> > +                    unsigned int *count,
> > +                    const char ***cpus)
> > +{
...
> > +    qemudParseCPUModels parse;
> > +
> > +    if (count)
> > +        *count = 0;
> > +    if (cpus)
> > +        *cpus = NULL;
> > +
...
> > +    if (len < 0) {
> > +        virReportSystemError(NULL, errno, "%s",
> > +                             _("Unable to read QEMU supported CPU models"));
> > +        goto cleanup;
> > +    }
> > +
> > +    if (parse(output, count, cpus) < 0) {
> 
> Can you put this in a nice namespace,  qemuParseCPUs or something
> along those lines.

This was discussed on IRC and "no" is the result. It's just a local
variable...

> > +static int
> > +qemudCapsInitCPU(virCapsPtr caps,
> > +                 const char *arch)
> > +{
> > +    virCPUDefPtr cpu = NULL;
> > +    union cpuData *data = NULL;
> > +    virNodeInfo nodeinfo;
> > +    int ret = -1;
> > +
> > +    if (VIR_ALLOC(cpu) < 0
> > +        || !(cpu->arch = strdup(arch))) {
> > +        virReportOOMError(NULL);
> > +        goto error;
> > +    }
> > +
> > +    if (nodeGetInfo(NULL, &nodeinfo))
> > +        goto error;
> > +
> > +    cpu->type = VIR_CPU_TYPE_HOST;
> > +    cpu->sockets = nodeinfo.sockets * nodeinfo.nodes;
> 
> I'm not sure this is the right thing todo here, since it means
> this data for 'sockets' differs from that shown by the nodeinfo
> command. If an app needs the total number of sockets, they can
> already calculate that either from the nodeinfo API, or from
> the NUMA topology data in the cpabilities, so we don't need to
> do it here too. Just make it match nodeinfo.sockets.

OK, makes sense.

> > @@ -832,8 +1027,17 @@ virCapsPtr qemudCapsInit(virCapsPtr old_caps) {
> >          VIR_WARN0("Failed to query host NUMA topology, disabling NUMA capabilities");
> >      }
> >  
> > +    if (old_caps == NULL || old_caps->host.cpu == NULL) {
> > +        if (qemudCapsInitCPU(caps, utsname.machine) < 0)
> > +            VIR_WARN0("Failed to get host CPU");
> > +    }
> > +    else {
> > +        caps->host.cpu = old_caps->host.cpu;
> > +        old_caps->host.cpu = NULL;
> > +    }
> > +
> >      virCapabilitiesAddHostMigrateTransport(caps,
> > -                                           "tcp");
> > +                                           "Tcp");
> 
> Why this change ? The migrate transport is the uri prefix, so this is
> changing the semantics.

Ah, it must have happened when rebasing to latest master and solving
conflicts. It looks like I hit ~ by accident or something like that. Thanks
for spotting that.

> > +    if (qemudProbeCPUModels(emulator, ut->machine, &ncpus, &cpus) < 0)
> > +        goto cleanup;
> > +
> > +    if (ncpus > 0 && host && def->cpu && def->cpu->model) {
> > +        virCPUCompareResult cmp;
> > +
> > +        cmp = cpuGuestData(conn, host, def->cpu, &data);
> > +        switch (cmp) {
> > +        case VIR_CPU_COMPARE_INCOMPATIBLE:
> > +            qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> > +                    "%s", _("guest CPU is not compatible with host CPU"));
> > +            /* fall through */
> > +        case VIR_CPU_COMPARE_ERROR:
> > +            goto cleanup;
> > +
> > +        default:
> > +            break;
> > +        }
> > +
> > +        if (VIR_ALLOC(guest) < 0 || !(guest->arch = strdup(ut->machine)))
> > +            goto no_memory;
> > +
> > +        if (qemudProbeCPUModels(emulator, ut->machine, &ncpus, &cpus) < 0
> > +            || cpuDecode(conn, guest, data, ncpus, cpus) < 0)
> > +            goto cleanup;
> 
> Can't we avoid calling Probe again here, since we alrady did this 20
> lines earlier. This second time will leak memory  from the first time.

Sure. The second call wasn't supposed to be there.

Thanks.

Jirka