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

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.