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

[Cole Robinson]

On 01/23/2019 07:08 AM, Andrea Bolognani wrote:
> On Tue, 2019-01-22 at 16:27 -0500, Cole Robinson wrote:
>> On 01/22/2019 12:39 PM, Cole Robinson wrote:
>>> On 01/21/2019 11:49 AM, Andrea Bolognani wrote:
>>>> 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...
> 
> I don't think that's going to be particularly controversial, and we
> should really make sure we get all the user-facing bits in at the
> same time IMHO, so I'd say go for it... If you're really unsure
> about it you can add that model in a separate patch right after this
> one, that way if someone happens not to like that we can drop it and
> otherwise we can squash them together before pushing.
> 

Alright will do

>>>> 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
> 
> I like it! I actually wanted to suggest something like that earlier,
> but for some reason I thought it would be more complicated than it
> turned out to be...
> 
> Better yet, you don't even need to add that switch statement to
> qemuBuildVirtioDevStr(): you can just use virDomainDeviceGetInfo()
> instead, thus making the code shorter and nicer :)
> 

Ah good catch, though the switch statement will end up in the final
result anyways with all the transitional additions

Thanks,
Cole