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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
