On Tue, 2016-05-17 at 16:10 -0400, John Ferlan wrote:
> >   * enabled and configure default values related to those features.
> >   */
> >  static void
> > -qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def)
> > +qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
> > +                                   virQEMUCapsPtr qemuCaps)
> >  {
> > -    switch (def->os.arch) {
> > -    case VIR_ARCH_ARMV7L:
> > -    case VIR_ARCH_AARCH64:
> > -        if (qemuDomainMachineIsVirt(def)) {
> > -            /* GIC is always available to ARM virt machines */
> > -            def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
> > +    do {
> 
> Not a fan...  Isn't what you're doing the same as:
> 
>     if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT &&
>         (def->os.arch == VIR_ARCH_ARMV7L || def->os.arch == VIR_ARCH_AARCH64) 
> &&
>         qemuDomainMachineIsVirt(def)) {

The idea was to closely mirror
virQEMUCapsFillDomainFeatureGICCaps(), but yeah, it's functionally
identical to your version.

I'm not exceedingly happy with cramming tons of conditions in the
same if() - I think it makes for less readable code overall - but
it's a well-established pattern used all over the place in libvirt,
so let's stick with it.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to