On 03/26/2017 10:22 PM, Lin Ma wrote:
> Create a network with a device pool containing all the VFs of an SR-IOV
> device.
>
I applied patches #1-3, thanks. Please split this into two patches: one with
the test/ and virtinst/ changes, the second with the ui/ and virtManager/
changes since those will take more work.
> Signed-off-by: Lin Ma <lma@xxxxxxxx>
> ---
> tests/xmlparse-xml/network-vf-pool-in.xml | 5 +
> tests/xmlparse-xml/network-vf-pool-out.xml | 6 ++
> tests/xmlparse.py | 21 ++++
> ui/createnet.ui | 92 ++++++++++++++++-
> virtManager/createnet.py | 153 ++++++++++++++++++++---------
> virtinst/network.py | 12 +++
> 6 files changed, 240 insertions(+), 49 deletions(-)
> create mode 100644 tests/xmlparse-xml/network-vf-pool-in.xml
> create mode 100644 tests/xmlparse-xml/network-vf-pool-out.xml
>
> diff --git a/virtManager/createnet.py b/virtManager/createnet.py
> index 9fdbc45..2154df1 100644
> --- a/virtManager/createnet.py
> +++ b/virtManager/createnet.py
> @@ -120,6 +120,12 @@ class vmmCreateNetwork(vmmGObjectUI):
> self.widget("header").modify_bg(Gtk.StateType.NORMAL, blue)
>
> # [ label, dev name ]
> + pf_list = self.widget("pf-list")
> + pf_model = Gtk.ListStore(str, str)
> + pf_list.set_model(pf_model)
> + uiutil.init_combo_text_column(pf_list, 0)
> +
> + # [ label, dev name ]
> fw_list = self.widget("net-forward")
> fw_model = Gtk.ListStore(str, str)
> fw_list.set_model(fw_model)
> @@ -167,6 +173,9 @@ class vmmCreateNetwork(vmmGObjectUI):
>
> self.widget("net-enable-ipv6-networking").set_active(False)
>
> + pf_model = self.widget("pf-list").get_model()
> + pf_model.clear()
> +
> fw_model = self.widget("net-forward").get_model()
> fw_model.clear()
> fw_model.append([_("Any physical device"), None])
> @@ -181,6 +190,26 @@ class vmmCreateNetwork(vmmGObjectUI):
> for name in devnames:
> fw_model.append([_("Physical device %s") % name, name])
>
> + devprettynames = []
> + ifnames = []
> + for pcidev in self.conn.filter_nodedevs("pci"):
> + if pcidev.xmlobj.capability_type == "virt_functions":
This is better for indentation:
if pcidev.xmlobj.capability_type != "virt_functions":
continue
[rest of code]
> + devdesc = pcidev.xmlobj.pretty_name()
> + for netdev in self.conn.filter_nodedevs("net"):
> + if pcidev.xmlobj.name == netdev.xmlobj.parent:
> + ifname = netdev.xmlobj.interface
> + devprettyname = "%s (%s)" % (ifname, devdesc)
> + devprettynames.append(devprettyname)
> + ifnames.append(ifname)
> + break
Indentation is weird here. You can use that 'continue' trick as well to reduce
some indentation
Note the example nodedev XML you added to tests/testdriver.xml doesn't hit
this condition, it probably needs a matching <interface> object as well. You
can test the testdriver with ./virt-manager --connect
test://`pwd`/tests/testdriver.xml
> + if not devprettynames:
> + pf_model.append([_("No available device"), None])
> + else:
> + for devprettyname, ifname in zip(devprettynames, ifnames):
> + pf_model.append([_("%s") % devprettyname, ifname])
> +
> + self.widget("pf-list").set_active(0)
> +
> self.widget("net-forward").set_active(0)
> self.widget("net-forward-mode").set_active(0)
> self.widget("net-forward-none").set_active(True)
> @@ -223,6 +252,11 @@ class vmmCreateNetwork(vmmGObjectUI):
> return self._get_network_helper("net-dhcpv6-end")
>
> def get_config_forwarding(self):
> + if self.widget("net-sriov-vfs-pool").get_active():
> + name = uiutil.get_list_selection(self.widget("pf-list"), column=1)
> + mode = "hostdev"
> + return [name, mode]
> +
> if self.widget("net-forward-none").get_active():
> return [None, None]
>
> @@ -481,10 +515,27 @@ class vmmCreateNetwork(vmmGObjectUI):
> self.widget("create-finish").grab_focus()
>
> def change_forward_type(self, ignore):
> - skip_fwd = self.widget("net-forward-none").get_active()
> + index = self.widget("pf-list").get_active()
> + model = self.widget("pf-list").get_model()
> + item = model[index]
> + if item[0] == "No available device":
You can't compare against the text here, since it's marked as translatable.
Instead check for item[1] is None
> + sriov_capable = False
> + else:
> + sriov_capable = True
> + self.widget("net-sriov-vfs-pool").set_sensitive(sriov_capable)
> + vf_pool = self.widget("net-sriov-vfs-pool").get_active()
I don't like naming these widgets 'sriov', it's better IMO to mirror libvirt
naming. So net-forward-mode-hostdev or something like that
> + if not vf_pool:
> + skip_fwd = self.widget("net-forward-none").get_active()
> +
> + self.widget("net-forward-mode").set_sensitive(not skip_fwd)
> + self.widget("net-forward").set_sensitive(not skip_fwd)
> + else:
> + self.widget("net-forward-mode").set_sensitive(False)
> + self.widget("net-forward").set_sensitive(False)
>
> - self.widget("net-forward-mode").set_sensitive(not skip_fwd)
> - self.widget("net-forward").set_sensitive(not skip_fwd)
> + self.widget("table36").set_sensitive(vf_pool)
> + self.widget("box13").set_sensitive(not vf_pool)
> + self.widget("hbox6").set_sensitive(not vf_pool)
>
If you are going to interact with any widgets in code, please give them
readable names in the ui files, not box13 etc.
> def change_ipv4_enable(self, ignore):
> enabled = self.get_config_ipv4_enable()
> @@ -676,56 +727,64 @@ class vmmCreateNetwork(vmmGObjectUI):
> net = self._build_xmlstub()
>
> net.name = self.widget("net-name").get_text()
> - net.domain_name = self.widget("net-domain-name").get_text() or None
> -
> - if self.widget("net-enable-ipv6-networking").get_active():
> - net.ipv6 = True
>
> dev, mode = self.get_config_forwarding()
> if mode:
> net.forward.mode = mode
> net.forward.dev = dev or None
>
> - if self.get_config_ipv4_enable():
> - ip = self.get_config_ip4()
> - ipobj = net.add_ip()
> - ipobj.address = str(ip.network + 1)
> - ipobj.netmask = str(ip.netmask)
> -
> - if self.get_config_dhcpv4_enable():
> - dhcpobj = ipobj.add_range()
> - dhcpobj.start = str(self.get_config_dhcpv4_start().network)
> - dhcpobj.end = str(self.get_config_dhcpv4_end().network)
> -
> - if self.get_config_ipv6_enable():
> - ip = self.get_config_ip6()
> - ipobj = net.add_ip()
> - ipobj.family = "ipv6"
> - ipobj.address = str(ip.network + 1)
> - ipobj.prefix = str(ip.prefixlen)
> -
> - if self.get_config_dhcpv6_enable():
> - dhcpobj = ipobj.add_range()
> - dhcpobj.start = str(self.get_config_dhcpv6_start().network)
> - dhcpobj.end = str(self.get_config_dhcpv6_end().network)
> -
> - netaddr = _make_ipaddr(self.get_config_routev4_network())
> - gwaddr = _make_ipaddr(self.get_config_routev4_gateway())
> - if netaddr and gwaddr:
> - route = net.add_route()
> - route.family = "ipv4"
> - route.address = netaddr.network
> - route.prefix = netaddr.prefixlen
> - route.gateway = gwaddr.network
> -
> - netaddr = _make_ipaddr(self.get_config_routev6_network())
> - gwaddr = _make_ipaddr(self.get_config_routev6_gateway())
> - if netaddr and gwaddr:
> - route = net.add_route()
> - route.family = "ipv6"
> - route.address = netaddr.network
> - route.prefix = netaddr.prefixlen
> - route.gateway = gwaddr.network
> + if net.forward.mode == "hostdev":
> + net.forward.managed = "yes"
> + pfobj = net.forward.add_pf()
> + pfobj.dev = net.forward.dev
> + net.forward.dev = None
> + net.domain_name = None
> + else:
> + net.domain_name = self.widget("net-domain-name").get_text() or None
> +
> + if self.widget("net-enable-ipv6-networking").get_active():
> + net.ipv6 = True
> +
> + if self.get_config_ipv4_enable():
> + ip = self.get_config_ip4()
> + ipobj = net.add_ip()
> + ipobj.address = str(ip.network + 1)
> + ipobj.netmask = str(ip.netmask)
> +
> + if self.get_config_dhcpv4_enable():
> + dhcpobj = ipobj.add_range()
> + dhcpobj.start = str(self.get_config_dhcpv4_start().network)
> + dhcpobj.end = str(self.get_config_dhcpv4_end().network)
> +
> + if self.get_config_ipv6_enable():
> + ip = self.get_config_ip6()
> + ipobj = net.add_ip()
> + ipobj.family = "ipv6"
> + ipobj.address = str(ip.network + 1)
> + ipobj.prefix = str(ip.prefixlen)
> +
> + if self.get_config_dhcpv6_enable():
> + dhcpobj = ipobj.add_range()
> + dhcpobj.start = str(self.get_config_dhcpv6_start().network)
> + dhcpobj.end = str(self.get_config_dhcpv6_end().network)
> +
> + netaddr = _make_ipaddr(self.get_config_routev4_network())
> + gwaddr = _make_ipaddr(self.get_config_routev4_gateway())
> + if netaddr and gwaddr:
> + route = net.add_route()
> + route.family = "ipv4"
> + route.address = netaddr.network
> + route.prefix = netaddr.prefixlen
> + route.gateway = gwaddr.network
> +
> + netaddr = _make_ipaddr(self.get_config_routev6_network())
> + gwaddr = _make_ipaddr(self.get_config_routev6_gateway())
> + if netaddr and gwaddr:
> + route = net.add_route()
> + route.family = "ipv6"
> + route.address = netaddr.network
> + route.prefix = netaddr.prefixlen
> + route.gateway = gwaddr.network
>
Please find a way to avoid indenting all this code. Move it to another
function or find a way to exit the function early in the hostdev case.
Thanks,
Cole
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
