Hi
----- Original Message -----
> On Mon, Nov 28, 2016 at 02:47:46PM +0400, marcandre.lureau@xxxxxxxxxx wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >
> > Similarly to virt-install --listen=none, add a combobox to select
> > the listen type: "address" or "none" for now, as suggested by Pavel
> > Hrdina.
>
> Hi, I'm sorry for the delay with review. This looks promising but there are
> some implementation details that should be addressed.
>
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> > ---
> > ui/gfxdetails.ui | 64
> > ++++++++++++++++++++++++++++++++++------------
> > virtManager/addhardware.py | 14 ++++++----
> > virtManager/details.py | 7 ++---
> > virtManager/domain.py | 11 +++++---
> > virtManager/gfxdetails.py | 38 ++++++++++++++++++++++-----
> > virtinst/devicegraphics.py | 16 +++++++++++-
> > 6 files changed, 115 insertions(+), 35 deletions(-)
>
> [...]
>
> > diff --git a/virtManager/addhardware.py b/virtManager/addhardware.py
> > index 7a3c8f2..3f868aa 100644
> > --- a/virtManager/addhardware.py
> > +++ b/virtManager/addhardware.py
> > @@ -1541,16 +1541,20 @@ class vmmAddHardware(vmmGObjectUI):
> >
> > def _validate_page_graphics(self):
> > try:
> > - (gtype, port,
> > - tlsport, addr, passwd, keymap, gl) =
> > self._gfxdetails.get_values()
> > + (gtype, port, tlsport, listen,
> > + addr, passwd, keymap, gl) = self._gfxdetails.get_values()
> >
> > self._dev = virtinst.VirtualGraphics(self.conn.get_backend())
> > self._dev.type = gtype
> > - self._dev.port = port
> > self._dev.passwd = passwd
> > - self._dev.listen = addr
> > - self._dev.tlsPort = tlsport
> > self._dev.gl = gl
> > +
> > + if not listen:
> > + self._dev.set_listen_none()
> > + else:
> > + self._dev.listen = addr
> > + self._dev.port = port
> > + self._dev.tlsPort = tlsport
>
> This only validates the graphics device, but I would prefer to do it
> properly,
> something like this:
>
> if not listen or listen == "none":
> self._dev.set_listen_none()
> elif listen == "address":
> self._dev.listen = addr
> self._dev.port = port
> self._dev.tlsPort = tlsport
> else
> raise ValueError(_("invalid listen type"))
Good catch
>
> > if keymap:
> > self._dev.keymap = keymap
> > except ValueError, e:
> > diff --git a/virtManager/details.py b/virtManager/details.py
> > index d803d52..d4e6629 100644
> > --- a/virtManager/details.py
> > +++ b/virtManager/details.py
> > @@ -2135,15 +2135,16 @@ class vmmDetails(vmmGObjectUI):
> > devobj=devobj)
> >
> > def config_graphics_apply(self, devobj):
> > - (gtype, port,
> > - tlsport, addr, passwd, keymap, gl) = self.gfxdetails.get_values()
> > + (gtype, port, tlsport, listen,
> > + addr, passwd, keymap, gl) = self.gfxdetails.get_values()
> >
> > kwargs = {}
> >
> > if self.edited(EDIT_GFX_PASSWD):
> > kwargs["passwd"] = passwd
> > if self.edited(EDIT_GFX_ADDRESS):
> > - kwargs["listen"] = addr
> > + kwargs["listen"] = listen
> > + kwargs["addr"] = addr
>
> Now that we support to select listen type it would be probably better to
> separate the listen from address. That would mean create all the bits as we
> already have for address.
I don't see much point in separating them, but ok
>
> > if self.edited(EDIT_GFX_KEYMAP):
> > kwargs["keymap"] = keymap
> > if self.edited(EDIT_GFX_PORT):
> > diff --git a/virtManager/domain.py b/virtManager/domain.py
> > index 6e742b9..f5159d6 100644
> > --- a/virtManager/domain.py
> > +++ b/virtManager/domain.py
> > @@ -824,7 +824,7 @@ class vmmDomain(vmmLibvirtObject):
> > self._redefine_xmlobj(xmlobj)
> >
> > def define_graphics(self, devobj, do_hotplug,
> > - listen=_SENTINEL, port=_SENTINEL, tlsport=_SENTINEL,
> > + listen=_SENTINEL, addr=_SENTINEL, port=_SENTINEL,
> > tlsport=_SENTINEL,
> > passwd=_SENTINEL, keymap=_SENTINEL, gtype=_SENTINEL,
> > gl=_SENTINEL):
> > xmlobj = self._make_xmlobj_to_define()
> > @@ -832,8 +832,8 @@ class vmmDomain(vmmLibvirtObject):
> > if not editdev:
> > return
> >
> > - if listen != _SENTINEL:
> > - editdev.listen = listen
> > + if addr != _SENTINEL:
> > + editdev.listen = addr
> > if port != _SENTINEL:
> > editdev.port = port
> > if tlsport != _SENTINEL:
> > @@ -846,6 +846,11 @@ class vmmDomain(vmmLibvirtObject):
> > editdev.type = gtype
> > if gl != _SENTINEL:
> > editdev.gl = gl
> > + if listen != _SENTINEL:
> > + if listen == 'none':
> > + editdev.set_listen_none()
> > + else:
> > + editdev.remove_listen_none()
> >
> > if do_hotplug:
> > self.hotplug(device=editdev)
> > diff --git a/virtManager/gfxdetails.py b/virtManager/gfxdetails.py
> > index f3cd3a9..d900b0b 100644
> > --- a/virtManager/gfxdetails.py
> > +++ b/virtManager/gfxdetails.py
> > @@ -50,8 +50,9 @@ class vmmGraphicsDetails(vmmGObjectUI):
> > "on_graphics_tlsport_auto_toggled": self._change_tlsport_auto,
> > "on_graphics_use_password": self._change_password_chk,
> >
> > + "on_graphics_listen_type_changed":
> > self._change_graphics_address,
> > "on_graphics_password_changed": lambda ignore:
> > self.emit("changed-password"),
> > - "on_graphics_address_changed": lambda ignore:
> > self.emit("changed-address"),
> > + "on_graphics_address_changed": self._change_graphics_address,
> > "on_graphics_tlsport_changed": lambda ignore:
> > self.emit("changed-tlsport"),
> > "on_graphics_port_changed": lambda ignore:
> > self.emit("changed-port"),
> > "on_graphics_keymap_changed": lambda ignore:
> > self.emit("changed-keymap"),
> > @@ -78,6 +79,14 @@ class vmmGraphicsDetails(vmmGObjectUI):
> > graphics_model.append(["spice", _("Spice server")])
> > graphics_model.append(["vnc", _("VNC server")])
> >
> > + graphics_listen_list = self.widget("graphics-listen-type")
> > + graphics_listen_model = Gtk.ListStore(str, str)
> > + graphics_listen_list.set_model(graphics_listen_model)
> > + uiutil.init_combo_text_column(graphics_listen_list, 1)
> > + graphics_listen_model.clear()
> > + graphics_listen_model.append(["address", _("Address")])
> > + graphics_listen_model.append(["none", _("None")])
> > +
> > self.widget("graphics-address").set_model(Gtk.ListStore(str, str))
> > uiutil.init_combo_text_column(self.widget("graphics-address"), 1)
> >
> > @@ -123,6 +132,7 @@ class vmmGraphicsDetails(vmmGObjectUI):
> > uiutil.set_grid_row_visible(self.widget("graphics-xauth"), False)
> >
> > self.widget("graphics-type").set_active(0)
> > + self.widget("graphics-listen-type").set_active(0)
> > self.widget("graphics-address").set_active(0)
> > self.widget("graphics-keymap").set_active(0)
> >
> > @@ -136,6 +146,7 @@ class vmmGraphicsDetails(vmmGObjectUI):
> > def get_values(self):
> > gtype = uiutil.get_list_selection(self.widget("graphics-type"))
> > port, tlsport = self._get_config_graphics_ports()
> > + listen =
> > uiutil.get_list_selection(self.widget("graphics-listen-type"))
> > addr = uiutil.get_list_selection(self.widget("graphics-address"))
> > keymap = uiutil.get_list_selection(self.widget("graphics-keymap"))
> > if keymap == "auto":
> > @@ -147,7 +158,7 @@ class vmmGraphicsDetails(vmmGObjectUI):
> >
> > gl = self.widget("graphics-opengl").get_active()
> >
> > - return gtype, port, tlsport, addr, passwd, keymap, gl
> > + return gtype, port, tlsport, listen, addr, passwd, keymap, gl
> >
> > def set_dev(self, gfx):
> > self.reset_state()
> > @@ -181,8 +192,12 @@ class vmmGraphicsDetails(vmmGObjectUI):
> > use_passwd = gfx.passwd is not None
> >
> > set_port("graphics-port", gfx.port)
> > - uiutil.set_list_selection(
> > - self.widget("graphics-address"), gfx.listen)
> > + if gfx.has_listen_none():
> > +
> > uiutil.set_list_selection(self.widget("graphics-listen-type"),
> > 'none')
> > + else:
> > +
> > uiutil.set_list_selection(self.widget("graphics-listen-type"),
> > 'address')
> > + uiutil.set_list_selection(
> > + self.widget("graphics-address"), gfx.listen)
> > uiutil.set_list_selection(
> > self.widget("graphics-keymap"), gfx.keymap or None)
> >
> > @@ -216,10 +231,15 @@ class vmmGraphicsDetails(vmmGObjectUI):
> > "graphics-tlsport-box", "graphics-opengl"]
> >
> > gtype = uiutil.get_list_selection(self.widget("graphics-type"))
> > + listen =
> > uiutil.get_list_selection(self.widget("graphics-listen-type"))
> > +
> > sdl_rows = ["graphics-xauth", "graphics-display"]
> > - vnc_rows = ["graphics-password-box", "graphics-address",
> > - "graphics-port-box", "graphics-keymap"]
> > - spice_rows = vnc_rows[:] + ["graphics-tlsport-box"]
> > + vnc_rows = ["graphics-password-box", "graphics-keymap"]
> > + if listen == 'address':
> > + vnc_rows.extend(["graphics-port-box", "graphics-address"])
> > + spice_rows = vnc_rows[:]
> > + if listen == 'address':
> > + spice_rows.extend(["graphics-tlsport-box"])
> > if self.conn.check_support(self.conn.SUPPORT_CONN_SPICE_GL):
> > spice_rows.extend(["graphics-opengl"])
> >
> > @@ -238,6 +258,10 @@ class vmmGraphicsDetails(vmmGObjectUI):
> > self._show_rows_from_type()
> > self.emit("changed-type")
> >
> > + def _change_graphics_address(self, ignore):
> > + self._show_rows_from_type()
> > + self.emit("changed-address")
> > +
> > def _change_port_auto(self, ignore):
> > self.widget("graphics-port-auto").set_inconsistent(False)
> > self._change_ports()
> > diff --git a/virtinst/devicegraphics.py b/virtinst/devicegraphics.py
> > index 07b554e..e885418 100644
> > --- a/virtinst/devicegraphics.py
> > +++ b/virtinst/devicegraphics.py
> > @@ -202,6 +202,11 @@ class VirtualGraphics(VirtualDevice):
> > self.remove_child(find_listen[0])
> > else:
> > find_listen[0].address = val
> > +
> > + if self.port is None and self.tlsPort is None and self.type ==
> > "spice":
> > + self.port = -1
> > + self.tlsPort = -1
> > +
> > return val
> > listen = XMLProperty("./@listen", set_converter=_set_listen)
> >
> > @@ -219,16 +224,25 @@ class VirtualGraphics(VirtualDevice):
> > for listen in self.listens:
> > self.remove_child(listen)
> >
> > + def remove_listen_none(self):
> > + for listen in self.listens:
> > + if listen.type == "none":
> > + self.remove_child(listen)
> > +
>
> This method is not strictly required because of the fact that if there is a
> listen type=none there should not be any other listen (libvirt will reject
> such
> XML) so it's safe to call remove_all_listens.
Ok (I remembered this is coming soon)
>
> > def add_listen(self):
> > obj = _GraphicsListen(self.conn)
> > self.add_child(obj)
> > return obj
> >
> > + def has_listen_none(self):
> > + return len(self.listens) > 0 and self.listens[0].type == "none"
> > +
>
> I would make this function more generic to get listen type of first listen
> element or None. So far only one listen type is supported in libvirt and
> virt-manager also supports only one listen type.
ok, I'll send a new series
thanks
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
