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

On 01/21/2019 11:49 AM, Andrea Bolognani wrote: > 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? > Yes I agree it would be nice to add that option. I suggest we make it a separate issue from this series though incase it's a contentious idea, v2 is already shaping up to be ~30 patches... > >> + 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. > Yeah it's hacky. Adding another arg here is going to add extra pain if when this is merged into qemuBuildVirtioStr, most callers are going have NULL arguments, and an increased chance of new future callers passing in incorrect values and accidentally triggering a wrong code path. I feel like this is another argument for the separated BuildTransitional or whatever, but I'm not sold either way so I'll stick with your suggestion