> > 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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list