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

[Andrea Bolognani]

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