[libvirt] [PATCH v5 10/13] qemu: Generate and use zPCI device in QEMU command line

[Andrea Bolognani]

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
> +char *
> +qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    virBufferAddLit(&buf, "zpci");
> +    virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci.uid);
> +    virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci.fid);
> +    virBufferAsprintf(&buf, ",target=%s", dev->alias);
> +    virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci.uid);

All of the above can be a single virBufferAsprintf() call.

[...]
> +static int
> +qemuAppendZPCIDevStr(virCommandPtr cmd,
> +                     virDomainDeviceInfoPtr dev)

I'd call this qemuCommandAddZPCIDevice() or something like that.

> +{
> +    char *devstr = NULL;
> +
> +    virCommandAddArg(cmd, "-device");

Empty line here.

> +    if (!(devstr = qemuBuildZPCIDevStr(dev)))
> +        return -1;

[...]
> +static int
> +qemuBuildExtensionCommandLine(virCommandPtr cmd,
> +                              virDomainDeviceInfoPtr dev)

Same comment about the naming as above.

> +{
> +    if (!virZPCIDeviceAddressIsEmpty(&dev->addr.pci.zpci))
> +        return qemuAppendZPCIDevStr(cmd, dev);
> +
> +    return 0;

I'd rather see this as

  if (virZPCIDeviceAddressIsEmpty(&dev->addr.pci.zpci))
      return 0;

  return qemuAppendZPCIDevStr(cmd, dev);

instead.

[...]
> @@ -4327,6 +4390,9 @@ qemuBuildSoundCommandLine(virCommandPtr cmd,
>          if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) {
>              virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL);
>          } else {
> +            if (qemuBuildExtensionCommandLine(cmd, &sound->info) < 0)
> +                return -1;

Do the codecs (handled immediately below) require a zpci device
as well? I figure they don't, but I also don't know much about
sound devices :)

[...]
> @@ -9086,6 +9171,9 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
>  
>      switch ((virDomainShmemModel)shmem->model) {
>      case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM:
> +        if (qemuBuildExtensionCommandLine(cmd, &shmem->info) < 0)
> +            return -1;
> +
>          devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps);
>          break;
>  
> @@ -9104,6 +9192,9 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
>  
>          ATTRIBUTE_FALLTHROUGH;
>      case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
> +        if (qemuBuildExtensionCommandLine(cmd, &shmem->info) < 0)
> +            return -1;
> +
>          devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps);
>          break;

This looks wrong: you should call qemuBuildExtensionCommandLine()
later on, like

  if (!devstr)
      return -1;

  if (qemuBuildExtensionCommandLine(cmd, &shmem->info) < 0)
    return -1;

  virCommandAddArgList(cmd, "-device", devstr, NULL);

instead.

-- 
Andrea Bolognani / Red Hat / Virtualization