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