Re: [libvirt] [RFC PATCH 08/12] qemu: introduce qemuBuildCPUDeviceStr
On Wed, 2015-01-21 at 10:24 +0100, Peter Krempa wrote:
On Wed, Jan 21, 2015 at 16:01:00 +0800, Zhu Guihua wrote:
> qemuBuildCPUDeviceStr being introduced is responsible for creating command
> line argument for '-device' for given cpu device.
>
> Signed-off-by: Zhu Guihua <zhugh.fnst(a)cn.fujitsu.com>
> ---
> src/qemu/qemu_command.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_command.h | 5 +++++
> 2 files changed, 55 insertions(+)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2ee3e10..824ad29 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5956,6 +5956,50 @@ static char *qemuBuildTPMDevStr(const virDomainDef *def,
> }
>
>
> +int
> +qemuBuildCPUDeviceStr(char **deviceStr,
> + virDomainCPUDefPtr dev,
> + virQEMUCapsPtr qemuCaps)
> +{
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU64_CPU)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("%s not supported in this QEMU binary"),
dev->driver);
You blindly assume that the user passed the correct model here and not
any other invalid string. As I've already said, having this converted to
an enum makes things easier.
> + goto error;
> + }
> +
> + virBufferAsprintf(&buf, "%s,id=%s,apic-id=%d",
> + dev->driver, dev->info.alias,
> + dev->apic_id);
Having a buffer for a single virBufferAsprintf case is a bit overkill.
> +
> + if (virBufferCheckError(&buf) < 0)
> + goto error;
> +
> + *deviceStr = virBufferContentAndReset(&buf);
> + return 0;
> +
> + error:
> + virBufferFreeAndReset(&buf);
> + return -1;
> +}
> +
> +static int
> +qemuBuildCPUDeviceCommandLine(virCommandPtr cmd,
> + virDomainCPUDefPtr dev,
> + virQEMUCapsPtr qemuCaps)
> +{
> + char *devstr = NULL;
> +
> + if (qemuBuildCPUDeviceStr(&devstr, dev, qemuCaps) < 0)
> + return -1;
> +
> + virCommandAddArgList(cmd, "-device", devstr, NULL);
> + VIR_FREE(devstr);
> + return 0;
> +}
> +
> +
> static char *qemuBuildSmbiosBiosStr(virSysinfoDefPtr def)
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> @@ -9862,6 +9906,12 @@ qemuBuildCommandLine(virConnectPtr conn,
> goto error;
> }
>
> + /* add cpu devices */
> + for (i = 0; i < def->ncpus; i++) {
> + if (qemuBuildCPUDeviceCommandLine(cmd, def->cpus[i], qemuCaps) < 0)
> + goto error;
> + }
As I've asked before. The question here is whether this is equivalent
with just increasing the vCPU count in the "-cpu" argument.
If not it will require a bit more work for setting cgroups, numa pinning
and other stuff.
Yes, we should require more work.
Regards,
Zhu
If they are equivalent we should use that instead and have the existing
code do the magic.
Peter