On 01/23/2019 10:40 AM, Andrea Bolognani wrote:
> On Tue, 2019-01-22 at 11:24 -0500, Cole Robinson wrote:
>> On 01/22/2019 07:03 AM, Andrea Bolognani wrote:
>>> On Mon, 2019-01-21 at 19:01 -0500, Cole Robinson wrote:
>>>>>> @@ -2290,8 +2295,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
>>>>>>
>>>>>> /* VirtIO RNG */
>>>>>> for (i = 0; i < def->nrngs; i++) {
>>>>>> - if (def->rngs[i]->model != VIR_DOMAIN_RNG_MODEL_VIRTIO ||
>>>>>> - !virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info))
>>>>>> + if (!virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info))
>>>>>> continue;
>>>>>
>>>>> I'm not convinced you can just remove the model check here...
>>>>> Shouldn't you extend it to include MODEL_VIRTIO_TRANSITIONAL and
>>>>> MODEL_VIRTIO_NON_TRANSITIONAL instead?
>>>>
>>>> It seemed redundant... the only enum value for rng model prior to this
>>>> patch was MODEL_VIRTIO, so any rng device here will by definition be
>>>> MODEL_VIRTIO. Same after this patch, but plus 2 more virtio models. It's
>>>> future proof if a non-PCI rng device model is added, but it causes more
>>>> work when a PCI rng device model is added, so it's a toss up
>>>
>>> When in doubt, I usually go for the safest / more explicit version.
>>
>> I mean, it's arguably safer and more explicit to duplicate every
>> DomainValidate device whitelist check in qemu_command.c too, but it
>> comes at the cost of more code and increased developer effort when
>> extending related code. In this particular case it took me extra time
>> when writing this code to figure out why the new rng models were not
>> generating the expected addresses.
>>
>> How about I add a prep patch that moves the RNG model check from
>> qemu_command.c into the qemu*Validate path, that way we are enforcing
>> earlier that no non-virtio RNG model can even enter this part of the
>> qemu driver.
>
> Looking at the code for rng and balloon more closely it's pretty
> clear that, unlike what happens with most other devices, they've
> been implemented with the assumption that there will only ever be
> one model, so I guess what you suggest above would already be an
> improvement over the status quo while preparing them for further
> models would be out of scope at best, pointless at worst... Just
> go with the above, then.
>
Okay, thank you. I'll also add a comment pointing out the situation,
along the lines of what fs models have:
/* Only support VirtIO-9p-pci so far. If that changes,
* we might need to skip devices here */
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
