Re: [libvirt] [PATCH 4/7] qemu: Default to GIC v2

On Sun, 2016-02-07 at 09:38 -0500, John Ferlan wrote: >> static int >> +qemuDomainDefAddDefaultFeatures(virDomainDefPtr def) > > ?qemuDomainDefSetDefaultFeatures? > > You're not adding gic, it's already added, you're just setting the > default version... although this could be unnecessary if host were the > default... Patch 5/7 actually enables the VIR_DOMAIN_FEATURE_GIC feature in the same function, so the name makes more sense after that one has been applied as well, but maybe I can keep the two steps separated and have both AddDefaultFeatures (for turning on features that should be turned on by default) and SetDefaultFeatures (for setting the specific value in cases like this, where it's not a simple boolean)? Or change the name to qemuDomainDefEnableDefaultFeatures() and still do both things in one place?

>> +{ >> + /* Default to GIC v2 if no version was specified */ >> + if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON && >> + def->gic_version == VIR_GIC_VERSION_NONE) >> + def->gic_version = VIR_GIC_VERSION_2; >> + >> + return 0; > > Since there is no other return value, this should be a void Okay, we can change the return type later if we start doing something that might fail. > BTW: Somewhere along the way docs/formatdomain.html.in needs an > adjustment to describe the options (host, 2, 3) and how they work. There is already some documentation for GIC in docs/formatdomain.html.in, but yeah, some improvements would definitely be a nice addition.