[PATCH 01/11] Add a button to bring up the Add Additional dialogs.

Chris Lumens clumens at redhat.com
Thu Apr 11 18:52:09 UTC 2013


> I want to look at the patches and do proper review. For now I just tried
> them and noticed that iscsi binding support is missing. Something we do in
> RHEL 6 like this:
> 
> http://rvykydal.fedorapeople.org/rhel64iscsibind.webm
> 
> User can choose if he wants to bind targets to specific devices (internally
> it affects what discovery returns). You can see ifaces in list
> of nodes (for non-bound case there is "default"). After the first login
> the choice (bind or not) can't be changed.

Ahh, I was not aware you could do this.  It was not included in the
design because we based it upon the older screenshots in the RHEL6
documentation, which did not include this checkbox.  Thanks for pointing
it out.

> I am attaching a patch adding the support on top your patches. The checkbox
> added to discovery tab should be probably UX-tuned. Also having the list
> of discovered nodes with interfaces each row per each node/iface combination
> is probably not the best:
> 
> Node        Iface
> -------------------
> target 1    p4p1
> target 1    p4p2
> target 2    p4p1
> target 2    p4p2
> target 3    p4p1

I'll have to apply your patch and play with this to really see what you
mean.

> Perhaps the choice should be done in 2 stages - target and interface, eg:
> 
> 
> Node        Iface
> -------------------
> target 1    ^p4p1    <- iface selection combobox in a cell
> target 2    ^p4p2.
> target 3    ^p4p1

I'd love if we could do things like this, but remember that the
CellRendererCombos don't display those little arrows by default so the
user will never be able to figure out there's something they can do with
that cell.  We had the same problem with the reclaim dialog.

> Also one minor thing you are probably aware of:
> Selecting Add iSCSI Target for 2nd time doesn't bring up the dialog unless
> there has been change in combobox since the 1st time.

Yeah, I know.  I haven't decided what to do about this yet.  I may just
want to replace it with a completely different widget.

> From 6e75cb279545f7045b26b28ca6bd7a9aea2cd3f9 Mon Sep 17 00:00:00 2001
> From: Radek Vykydal <rvykydal at redhat.com>
> Date: Thu, 11 Apr 2013 11:11:53 +0200
> Subject: [PATCH] Add support ford iSCSI iface binding.
> 
> ---
>  pyanaconda/ui/gui/spokes/advstorage/iscsi.glade | 29 +++++++++++++++++++++++++
>  pyanaconda/ui/gui/spokes/advstorage/iscsi.py    | 21 ++++++++++++++++--
>  2 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/pyanaconda/ui/gui/spokes/advstorage/iscsi.glade b/pyanaconda/ui/gui/spokes/advstorage/iscsi.glade
> index 34b0c41..977f955 100644
> --- a/pyanaconda/ui/gui/spokes/advstorage/iscsi.glade
> +++ b/pyanaconda/ui/gui/spokes/advstorage/iscsi.glade
> @@ -643,6 +643,22 @@
>                        </object>
>                        <packing>
>                          <property name="left_attach">0</property>
> +                        <property name="top_attach">7</property>
> +                        <property name="width">3</property>
> +                        <property name="height">1</property>
> +                      </packing>
> +                    </child>
> +                    <child>
> +                      <object class="GtkCheckButton" id="bindCheckbutton">
> +                        <property name="label" translatable="yes">Bind targets to network interfaces</property>
> +                        <property name="visible">True</property>
> +                        <property name="can_focus">True</property>
> +                        <property name="receives_default">False</property>
> +                        <property name="xalign">0</property>
> +                        <property name="draw_indicator">True</property>
> +                      </object>
> +                      <packing>
> +                        <property name="left_attach">0</property>
>                          <property name="top_attach">6</property>
>                          <property name="width">3</property>
>                          <property name="height">1</property>

Remember to make the checkbutton accessible with the keyboard.

> @@ -243,6 +250,15 @@ class ISCSIDialog(GUIObject):
>          discoveredLabel.set_markup(discoveredLabel.get_label() % {"initiatorName": credentials.initiator,
>                                                                    "targetAddress": credentials.targetIP})
>  
> +        bind = self._bindCheckbox.get_active()
> +        if self.iscsi.mode == "none" and not bind:
> +            self.iscsi.delete_interfaces()
> +        elif (self.iscsi.mode == "bind"
> +              or self.iscsi.mode == "none" and bind):
> +            activated = set(nm.nm_activated_devices())
> +            created = set(self.iscsi.ifaces.values())
> +            self.iscsi.create_interfaces(activated - created)
> +
>          spinner = self.builder.get_object("waitSpinner")
>          spinner.start()
>  

Does this take any time at all?  If so, it should go into _discover.
Also, do you need to convert activated-created into a list before
passing it to create_interfaces?

- Chris


More information about the anaconda-patches mailing list