Re: [libvirt] [PATCH 03/18] qemu: Support disk model=virtio-{non-}transitional

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?

> [...] > > @@ -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