[libvirt] [PATCH 03/18] qemu: Support disk model=virtio-{non-}transitional

[Cole Robinson]

On 01/22/2019 06:53 AM, Andrea Bolognani wrote:
> On Mon, 2019-01-21 at 17:40 -0500, Cole Robinson wrote:
>> On 01/18/2019 07:33 AM, Andrea Bolognani wrote:
>>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
>>>> +static int
>>>> +qemuBuildVirtioTransitional(virBufferPtr buf,
>>>> +                            const char *baseName,
>>>> +                            virQEMUCapsPtr qemuCaps,
>>>> +                            virDomainDeviceAddressType type,
>>>> +                            int model,
>>>> +                            virDomainDeviceType devtype)
>>>> +{
>>>> +    int tmodel_cap, ntmodel_cap;
>>>> +    bool has_tmodel, has_ntmodel;
>>>> +
>>>> +    if (qemuBuildVirtioDevStr(buf, baseName, type) < 0)
>>>> +        return -1;
>>>> +
>>>> +    switch (devtype) {
>>>> +        case VIR_DOMAIN_DEVICE_DISK:
>>>> +            has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;
>>>> +            has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;
>>>> +            tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL;
>>>> +            ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL;
>>>> +            break;
>>>
>>> I like this approach much better than the one used in the RFC,
>>> eg. passing two booleans to the function. Nice!
>>>
>>> What I don't like is that you're building this fairly thin wrapper
>>> around qemuBuildVirtioDevStr() when IMHO you should rather be
>>> agumenting the original function - mostly because the new name is
>>> not nearly as good :) Do you think you could make that happen?
>>
>> Hmm, seems weird to make call sites that will never support the
>> transitional naming (virtio-keyboard/mouse/tablet/gpu) have to call into
>> this function, passing DEVICE_TYPE etc. I can split the transitional
>> bits into their own function and not have it wrap BuildVirtioDevStr but
>> be a follow on, so for example:
>>
>> if (qemuBuildVirtioDevStr(...) < 0)
>>     goto error;
>> if (qemuBuildVirtioTransitionalStr(...) < )
>>     goto error;
>>
>> Does that work?
> 
> The split version is more likely to end up being misused, so I
> wouldn't go down that road.
> 
> As for the naming, my suggestion was to stick with the original
> qemuBuildVirtioDevStr() name, add parameters to it, and not
> introduce qemuBuildVirtioTransitional() at all, so it wouldn't look
> weird when called for devices that are modern only.
> 
> I don't see a problem with passing devType even when you're not
> ultimately going to make decisions based on it for certain devices.
> 

Okay I'll go that route

>>> [...]
>>>> @@ -723,6 +723,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>>>>      case VIR_DOMAIN_DEVICE_DISK:
>>>>          switch ((virDomainDiskBus) dev->data.disk->bus) {
>>>>          case VIR_DOMAIN_DISK_BUS_VIRTIO:
>>>> +            if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL)
>>>> +                return pciFlags;
>>>
>>> Perhaps a short comment about how transitional VirtIO devices can
>>> only be plugged into conventional PCI slots would be appropriate
>>> here, for the benefit of those looking at the code years from now :)
>>
>> This same pattern is duplicated in the rest of the series, seems weird
>> to comment one site, but commenting all of them is overkill. I guess
>> commenting one site is the middle ground unless you have a better idea
>> of where to put the comment
> 
> I don't think having a short comment such as
> 
>   /* Transitional devices only work in conventional PCI slots */
> 
> repeated a few times really counts as overkill :) So I'd go that way.
> 

Cool, I'll add it to the series

Thanks,
Cole