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. -- Andrea Bolognani / Red Hat / Virtualization