On Mon, 2019-01-21 at 17:40 -0500, Cole Robinson wrote:
> On 01/18/2019 07:33 AM, Andrea Bolognani wrote:
> > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
> > > +static int
> > > +qemuBuildVirtioTransitional(virBufferPtr buf,
> > > + const char *baseName,
> > > + virQEMUCapsPtr qemuCaps,
> > > + virDomainDeviceAddressType type,
> > > + int model,
> > > + virDomainDeviceType devtype)
> > > +{
> > > + int tmodel_cap, ntmodel_cap;
> > > + bool has_tmodel, has_ntmodel;
> > > +
> > > + if (qemuBuildVirtioDevStr(buf, baseName, type) < 0)
> > > + return -1;
> > > +
> > > + switch (devtype) {
> > > + case VIR_DOMAIN_DEVICE_DISK:
> > > + has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;
> > > + has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;
> > > + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL;
> > > + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL;
> > > + break;
> >
> > I like this approach much better than the one used in the RFC,
> > eg. passing two booleans to the function. Nice!
> >
> > What I don't like is that you're building this fairly thin wrapper
> > around qemuBuildVirtioDevStr() when IMHO you should rather be
> > agumenting the original function - mostly because the new name is
> > not nearly as good :) Do you think you could make that happen?
>
> Hmm, seems weird to make call sites that will never support the
> transitional naming (virtio-keyboard/mouse/tablet/gpu) have to call into
> this function, passing DEVICE_TYPE etc. I can split the transitional
> bits into their own function and not have it wrap BuildVirtioDevStr but
> be a follow on, so for example:
>
> if (qemuBuildVirtioDevStr(...) < 0)
> goto error;
> if (qemuBuildVirtioTransitionalStr(...) < )
> goto error;
>
> Does that work?
The split version is more likely to end up being misused, so I
wouldn't go down that road.
As for the naming, my suggestion was to stick with the original
qemuBuildVirtioDevStr() name, add parameters to it, and not
introduce qemuBuildVirtioTransitional() at all, so it wouldn't look
weird when called for devices that are modern only.
I don't see a problem with passing devType even when you're not
ultimately going to make decisions based on it for certain devices.
> > [...]
> > > @@ -723,6 +723,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
> > > case VIR_DOMAIN_DEVICE_DISK:
> > > switch ((virDomainDiskBus) dev->data.disk->bus) {
> > > case VIR_DOMAIN_DISK_BUS_VIRTIO:
> > > + if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL)
> > > + return pciFlags;
> >
> > Perhaps a short comment about how transitional VirtIO devices can
> > only be plugged into conventional PCI slots would be appropriate
> > here, for the benefit of those looking at the code years from now :)
>
> This same pattern is duplicated in the rest of the series, seems weird
> to comment one site, but commenting all of them is overkill. I guess
> commenting one site is the middle ground unless you have a better idea
> of where to put the comment
I don't think having a short comment such as
/* Transitional devices only work in conventional PCI slots */
repeated a few times really counts as overkill :) So I'd go that way.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
