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

On Mon, 2019-01-21 at 19:01 -0500, Cole Robinson wrote: > 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 I'm not sure I would qualify this style as "recommended", but I certainly prefer it and there are instances of the pattern around the codebase, so I'll switch to it whenever I happen to be touching a VIR_ENUM_IMPL() call. I haven't had any trouble getting such changes past reviewers so far :)

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