
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. Sure. [...] +static int +qemuAppendZPCIDevStr(virCommandPtr cmd, + virDomainDeviceInfoPtr dev) I'd call this qemuCommandAddZPCIDevice() or something like that. OK.
+{ + char *devstr = NULL; + + virCommandAddArg(cmd, "-device"); Empty line here. Yeah.
+ if (!(devstr = qemuBuildZPCIDevStr(dev))) + return -1; [...] +static int +qemuBuildExtensionCommandLine(virCommandPtr cmd, + virDomainDeviceInfoPtr dev) Same comment about the naming as above. OK.
+{ + 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. How about using switch? I think extension flag should be check and
在 2018/9/11 下午10:31, Andrea Bolognani 写道: then build command for each case although there's only zpci case.
[...]
@@ -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 :)
If its address is PCI, we should add a zpci for it. zPCI is not related to its functionality.
[...]
@@ -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.
Good catch. Thanks for your comments. -- Yi Min