On Mon, Nov 19, 2012 at 03:04:38PM +0000, Christoffer Dall wrote:
> On Mon, Nov 19, 2012 at 9:55 AM, Will Deacon <will.deacon@xxxxxxx> wrote:
> > On Sat, Nov 10, 2012 at 03:42:59PM +0000, Christoffer Dall wrote:
> >> +int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level)
> >> +{
> >> + u32 irq = irq_level->irq;
> >> + unsigned int irq_type, vcpu_idx, irq_num;
> >> + int nrcpus = atomic_read(&kvm->online_vcpus);
> >> + struct kvm_vcpu *vcpu = NULL;
> >> + bool level = irq_level->level;
> >> +
> >> + irq_type = (irq >> KVM_ARM_IRQ_TYPE_SHIFT) & KVM_ARM_IRQ_TYPE_MASK;
> >> + vcpu_idx = (irq >> KVM_ARM_IRQ_VCPU_SHIFT) & KVM_ARM_IRQ_VCPU_MASK;
> >> + irq_num = (irq >> KVM_ARM_IRQ_NUM_SHIFT) & KVM_ARM_IRQ_NUM_MASK;
> >> +
> >> + trace_kvm_irq_line(irq_type, vcpu_idx, irq_num, irq_level->level);
> >> +
> >> + if (irq_type == KVM_ARM_IRQ_TYPE_CPU ||
> >> + irq_type == KVM_ARM_IRQ_TYPE_PPI) {
> >> + if (vcpu_idx >= nrcpus)
> >> + return -EINVAL;
> >> +
> >> + vcpu = kvm_get_vcpu(kvm, vcpu_idx);
> >> + if (!vcpu)
> >> + return -EINVAL;
> >> + }
> >> +
> >> + switch (irq_type) {
> >> + case KVM_ARM_IRQ_TYPE_CPU:
> >> + if (irq_num > KVM_ARM_IRQ_CPU_FIQ)
> >> + return -EINVAL;
> >> +
> >> + return vcpu_interrupt_line(vcpu, irq_num, level);
> >> + }
> >> +
> >> + return -EINVAL;
> >> +}
> >
> > Holy cyclomatic complexity Batman! Any way this can be cleaned up?
> >
> you mean the interface or the implementation of kvm_vm_ioctl_irq_line?
> If the latter, there's just a lot of bits to decode here.
I just think that this function is incredibly hard to read: different nested
conditions under duplicate checks of the same variables which differ between
an if(...) and a switch(...). I appreciate that it's a complex problem,
which is why I helpfully didn't suggest an alternative! There must be
*something* we can do though.
Will
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
