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

[Cole Robinson]

On 01/22/2019 12:39 PM, Cole Robinson wrote:
> 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
> 

What do you think of this approach? See the attached two patches. It
adds a domain_conf.c function virDomainDeviceSetData which makes it
easier to create a virDomainDeviceDef. qemuBuildVirtioDevStr callers now
pass in a virDomainDeviceType and the specific DefPtr for their device
(virDomainDiskDefPtr, virDomainNetDefPtr etc). qemuBuildVirtioDevStr
then turns that into a virDomainDeviceDef and acts on that locally.

Saves having to extend the argument list several times (like this
example above, and the net->model string example). Seperately I think
virDomainDeviceSetData can be used to clean up some device interactions
elsewhere too

Thanks,
Cole
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-qemu-command-Make-BuildVirtioDevStr-more-generic.patch
Type: text/x-patch
Size: 10434 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190122/f93ccb37/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-conf-Add-virDomainDeviceSetData.patch
Type: text/x-patch
Size: 4976 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190122/f93ccb37/attachment-0003.bin>