Re: [libvirt] [PATCH v2 12/25] qemu: Support disk model=virtio-{non-}transitional
On Wed, 2019-02-06 at 12:17 -0500, Cole Robinson wrote:
On 1/29/19 5:25 AM, Andrea Bolognani wrote:
> On Wed, 2019-01-23 at 16:32 -0500, Cole Robinson wrote:
> [...]
> > + switch (devtype) {
> > + case VIR_DOMAIN_DEVICE_DISK:
> > + has_tmodel = device.data.disk->model ==
VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;
> > + has_ntmodel = device.data.disk->model ==
VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;
> > + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_TRANSITIONAL;
> > + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_NON_TRANSITIONAL;
> > + break;
>
> I wonder if this would look slightly nicer as
>
> case VIR_DOMAIN_DEVICE_DISK: {
> virDomainDiskDefPtr disk = (virDomainDiskDefPtr) devdata;
>
> has_tmodel = disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;
> has_ntmodel = disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;
> tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_TRANSITIONAL;
> ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_NON_TRANSITIONAL;
>
> break;
> }
>
> but up to you, really.
Makes for shorter lines which is nice but kind of offsets the benefit of
converting to virDomainDeviceSetData in the first place...
A bit, yes: if you did this, then the only use you'd get out of
the virDomainDeviceDef you just reconstructed would be getting a
virDomainDeviceInfo out of it... On the other hand, that also means
you wouldn't be basically open-coding virDomainDeviceGetInfo() in
yet another place.
As I said, pick whichever you like the most :)
--
Andrea Bolognani / Red Hat / Virtualization