Re: [libvirt] [PATCH 2/2] qemu: Unify generation of command line for virtio devices
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