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

On 01/21/2019 08:13 AM, Andrea Bolognani wrote: > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > [...] > > VIR_ENUM_IMPL(virDomainRNGModel, > > VIR_DOMAIN_RNG_MODEL_LAST, > > - "virtio"); > > + "virtio", > > + "virtio-transitional", > > + "virtio-non-transitional"); > > Since you're touching the definition, you can make it > > VIR_ENUM_IMPL(virDomainRNGModel, > ... > "virtio-non-transitional", > ); > > so that if and when we'll need to add another model the diff > will look nicer. Okay I'll adjust it. FWIW I like this style better but it's not well represented in domain_conf.c. I explicitly didn't make a change like this before posting because I had no idea if it would generate a complaint or not. IMO if this is the recommended style then we should fix all instances in one sweep and add a syntax-check for it

> [...] > > @@ -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