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