Re: [libvirt] [PATCH v3 12/13] Implement CPU selection in QEMU driver
> 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