Re: [libvirt] [PATCH 08/18] qemu: Support rng model=virtio-{non-}transitional

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.