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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
