Code review comment for lp:~tr3buchet/nova/xs_multi_nic

Nice work, Trey. Here are my thoughts so far (mostly style stuff):

> 9 + def spawn(self, instance, network_info=None):

Not exposed from xenapi_conn.py?

Are all virt-drivers going to accept a network_info=None kwarg?

> 25 + if network_info is not None:
> 26 + self.inject_network_info(instance, network_info)
> 27 + self.create_vifs(instance, [nw for (nw, mapping) in network_info])
> 28 + else:
> 29 + # TODO(tr3buchet) - goes away with multi-nic
> 30 + networks = self.inject_network_info(instance)
> 31 + self.create_vifs(instance, networks)

It seems like we should be able to simplify all of that to:

    networks = self.inject_network_info(instance, network_info)
    self.create_vifs(instance, networks)

Assuming `inject_network_info` returns `networks` correctly for both the
network_info=None and network_info!=None cases.

> def inject_network_info(self, instance, network_info=None):

This is method is handling two separate cases 1) multi_nic and 2) no
multi_nice. Since it's pretty long, I think it's a good canidate to be broken
up into two smaller methods, something like:

    def inject_network_info(self, instance, network_info=None):
        if network_info:
            networks = self.inject_network_info_multi_nic(self, instance, network_info)
        else:
            networks = self.inject_network_info_default(self, instance)

        return networks

    def inject_network_info_multi_nic(self, instance, network_info):
        # multi-nic stuff here
        return networks

    def inject_network_info_default(self, instance):
        # old code here
        return networks

If possible, it's a good idea to have the inject_network_info stuff continue
to return `networks`; this is more consistent and allows you to make the
simplification noted above.

> 59 + self.write_to_param_xenstore(vm_ref, {location: mapping})

Syntax error. {'location': mapping}

> 61 + self.write_to_xenstore(vm_ref, location,
> 62 + mapping['location'])

Appears misaligned. Seems like we have to 'accepted' conventions here:

1. Line-up with the first arg
    self.write_to_xenstore(vm_ref, location,
                           mapping['location'])

2. +1 level hanging indent
    self.write_to_xenstore(vm_ref, location,
        mapping['location'])
OR

    self.write_to_xenstore(
        vm_ref, location, mapping['location'])

Looks like there's an opportunity to DRY up the code a bit. Looks like both
multi-nic and default both do something like

    for network in networks:
        try:
            self.write_to_xenstore(vm_ref, location,
                                           mapping['location'])
        except KeyError:
            # catch KeyError for domid if instance isn't running
            pass

There should be a way of extracting that out.