On Tue, 2018-09-04 at 16:09 +0200, Ján Tomko wrote:
On Fri, Aug 31, 2018 at 04:03:10PM +0200, Andrea Bolognani wrote:
+static char* +qemuBuildVirtioDevStr(const virDomainDeviceInfo *info, + const char *baseName) [...] + virBufferAsprintf(&buf, "%s-%s", baseName, implName);
buf is used exactly once in this function, could have been just virAsprintf.
Or, even better, since all the calls are followed by adding the string to a buffer, just pass the buffer as the function argument.
I did it that way initially, but then I changed it to return a char* to be consistent with other qemuBuild*DevStr(). I can definitely change it back, but perhaps a different name would be more appropriate at that point. [...]
+ if (!(devStr = qemuBuildVirtioDevStr(&(disk->info), "virtio-blk")))
&disk->info is enough.
Or even simpler: disk->info.type
Okay, I'll go with the latter. [...]
+ if (disk->iothread && + (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW || + disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { + virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread);
The iothread change could be separated for an easier-to-read patch.
Agreed, it should have been that way to begin with.
Also, formatting it unconditionally would be nicer - do we even need to check for the address type? We format it unconditionally for the virtio-scsi controller.
Not sure. I don't really know anything about iothreads, so I purposefully steered clear of changing that part :) [...]
+ virBufferAddStr(&buf, devStr); + VIR_FREE(devStr); + } else { + virBufferAddStr(&buf, nic);
virBufferAddStr(&buf, net->model);
Since we no longer use 'nic' unconditionally.
Okay. [...]
case VIR_DOMAIN_INPUT_TYPE_TABLET: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TABLET) || - (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-tablet is not supported by this QEMU binary")); - goto error; - }
The capability check refactor is out of scope of this patch. Also, they should be in the *Validate functions, not the command line formatter.
Agreed, I'll split that part off. [...]
case VIR_DOMAIN_INPUT_TYPE_LAST: - break; + default: + virReportEnumRangeError(virDomainInputType, + dev->type); + goto error; + }
Unrelated virReportEnumRangeError addition.
I'll make that a separate patch too. [...]
+ if (dev->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { + virBufferAddLit(&buf, ",evdev="); + virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev);
Personally, I'd also separate all the changes that remove the additional attributes from the device name.
Agreed. [...]
- if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "virtio-rng-ccw,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); - else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - virBufferAsprintf(&buf, "virtio-rng-s390,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); - else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - virBufferAsprintf(&buf, "virtio-rng-device,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias);
Oh, fun. Not only we merged this copy & paste rng= attribute addition, it was later amended to add the id= attribute. Then broken into separate lines by a separate commit.
Well, at least now we're going to get rid of the duplication :) -- Andrea Bolognani / Red Hat / Virtualization