Re: [libvirt] [PATCH 16/18] qemu: Support scsi controller model=virtio-{non-}transitional

On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > Add <controller type='scsi' model handling for virtio transitional > devices. Ex: > > <controller type='scsi' model='virtio-transitional'/> > > * "virtio-transitional" maps to qemu "virtio-scsi-pci-transitional" > * "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional" > > The naming here doesn't match the pre-existing model=virtio-scsi. > The prescence of '-scsi' there seems kind of redundant as we have > type='scsi' already, so I decided to follow the pattern of other > patches and use virtio-transitional etc. Completely agree with the rationale here; however, in order to really match the way other devices are handled, I think we should make it so you can use <controller type='scsi' model='virtio'/> as well, which of course would behave the same as the currently available version. What do you think?

> + case VIR_DOMAIN_DEVICE_CONTROLLER: > + if (strstr(baseName, "scsi")) { > + has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL; > + has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL; > + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL; > + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL; > + break; > + } > + return 0; Using strstr() here is kinda crude, especially since the caller has enough information to pass the appropriate virDomainControllerType value, both in this case and later on for serial controllers. I'd say just add yet another argument to the function. Yeah, it starts to get quite unsightly, but I'd really rather not resort to string comparison when a nice, type safe enum will do.