[et-mgmt-tools] [PATCH] Resend of multiple nic patch

[David Lutterkort]

On Tue, 2008-08-05 at 08:15 -0400, bkearney at redhat.com wrote:
> # HG changeset patch
> # User bkearney at localhost.localdomain
> # Date 1217876425 14400
> # Node ID 3507f83147d566fb690dc9b87c0dbb35cd9e6f4c
> # Parent  6a207373b908ab521d33cd675c7c8d3854bdc1f1
> multiple nic support for virt-image. Added support to allow multiple
> interface elements in the virt-image.xml. The command line can specify
> any number of -w or -b elements and the tool will add default networks
> up to the number of nics specified. It is assumbed that eth0 is the first
> item specified.

Looks good. Two small nits:

> diff -r 6a207373b908 -r 3507f83147d5 virt-image
> --- a/virt-image	Tue Jul 29 11:21:07 2008 -0400
> +++ b/virt-image	Mon Aug 04 15:00:25 2008 -0400
> @@ -59,17 +59,14 @@
>      cli.get_vcpus(vcpus, check_cpu, guest, conn)
>  
>  def get_networks(domain, macs, bridges, networks, guest):
> -    (macs, networks) = cli.digest_networks(macs, bridges, networks)
> -
> -    nnics = 0
> -    if domain.interface:
> -        nnics = 1
> +    nnics = domain.interface
> +    (macs, networks) = cli.digest_networks(macs, bridges, networks, nnics)
>  
>      if nnics == 0 and len(networks) > 0:
>          print >> sys.stderr, _("Warning: image does not support networking, ignoring network related options")
>          return

This check should be 'if len(networks) > nnics' and then warn that the
last len(networks) - nnics network specs will be ignored.

> diff -r 6a207373b908 -r 3507f83147d5 virtinst/cli.py
> --- a/virtinst/cli.py	Tue Jul 29 11:21:07 2008 -0400
> +++ b/virtinst/cli.py	Mon Aug 04 15:00:25 2008 -0400
> @@ -262,41 +262,41 @@
>          fail(_("Unknown network type ") + network)
>      guest.nics.append(n)
>  
> -def digest_networks(macs, bridges, networks):
> +def digest_networks(macs, bridges, networks, nics = 1):
>      if type(bridges) != list and bridges != None:
>          bridges = [ bridges ]
>  
>      if type(macs) != list and macs != None:
>          macs = [ macs ]
> +    elif macs is None:
> +        macs = []
>  
>      if type(networks) != list and networks != None:
>          networks = [ networks ]
> +    elif networks is None:
> +        networks = []

It would be clearer to write these two as

        if macs is None:
        	macs = []
        elif type(macs) != list
        	macs = [ macs ]
        
same for networks

David