On Fri, Jan 21, 2022 at 09:20:42AM -0800, Andrea Bolognani wrote:
> On Fri, Jan 21, 2022 at 04:57:34PM +0000, Daniel P. Berrangé wrote:
> > On Mon, Jan 17, 2022 at 11:52:48AM +0100, Andrea Bolognani wrote:
> > > +static const char *
> > > +virQEMUCapsAccelStr(virDomainVirtType type)
> > > +{
> > > + if (type == VIR_DOMAIN_VIRT_QEMU)
> > > + return "tcg";
> > > +
> > > + return virDomainVirtTypeToString(type);
> > > +}
> >
> > I don't like this use of virDomainVirtTypeToString since the
> > vast majority of values it returns are not valid QEMU accelerators.
> >
> > I'd say we should just directly return the virt types we actually
> > expect to get - the invalid ones will have already been filtered
> > out. ie
> >
> > virQEMUCapsAccelStr(virDomainVirtType type)
> > {
> > if (type == VIR_DOMAIN_VIRT_KVM)
> > return "kvm";
> > else
> > return "tcg";
> > }
> >
> > that is still easily extended to add hvf and so on, without being
> > misleading about supporting any virt type
>
> The original version works because, as you note, we already make sure
> much earlier[1] that only values that correspond to valid QEMU
> accelerators can be used, but I'm okay with being more explicit.
>
> I'd rather use a switch statement though, that way next time an
> accelerator has to be added there will be fewer spots where we can
> easily forget to add handling for it. Does that sound good?
I don't mind.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
