Code review comment for lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu

Revision history for this message
Soren Hansen (soren) wrote :

> A couple of tiny nits:
>
> > 30 + if len(arch_nodes):
>
> Usually better to just write:
>
> if arch_nodes:

The reasoning was that I'm not just referencing it afterwards, I'm specifically indexing into it. To ensure that that is a valid operation, it needs to have a non-zero length, not just be any value that evaluates to True.

> > 56 + while topology_node != None:
> Comparisons to None should use identity not equivalence:

You are quite right. I just didn't go into this to fix existing style issues (I did not write that comparison), but rather just to fix a specific bug.

> while topology_node is not None:
>
> or if its None-ness doesn't matter:
>
> while topology_node:

Right.

I'll fix this as well as the thing Brian points out in a separate patch.

> > 8 - if FLAGS.connection_type != 'libvirt':
> > 9 + if (FLAGS.connection_type != 'libvirt' or
> > 10 + (FLAGS.connection_type == 'libvirt' and
> > 11 + FLAGS.libvirt_type not in ['kvm', 'qemu'])):
> > 12 msg = _('Only KVM is supported for now. Sorry!')
> > 13 raise exception.Error(msg)

> The logic differs from the logging. Is live-migration supported for both KVM
> and QEMU now? If so, we should probably update the logging to reflect that:
> "Only KVM or QEMU is supported.."

Very good point. I do believe QEMu is migratable as well. I'll update the error message.

« Back to merge proposal