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?
[1] In qemuValidateDomainDef() via virQEMUCapsIsVirtTypeSupported()
--
Andrea Bolognani / Red Hat / Virtualization
