[libvirt] [RFC PATCH 08/12] qemu: introduce qemuBuildCPUDeviceStr

[Zhu Guihua]

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 at 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