[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH rhel6-branch 1/7] iscsi: add iface binding support to discovery and setup GUI (#500273)



Ack, with comments below.

On Tue, Feb 28, 2012 at 12:03:28PM +0100, Radek Vykydal wrote:
> Resolves: rhbz#500273
> 
> In this initial support all used nodes will be either iface bound
> using default iface.
> ---
>  iw/advanced_storage.py |   18 ++++++++++++++++--
>  storage/iscsi.py       |   43 +++++++++++++++++++++++++++++++++++++++++++
>  ui/adddrive.glade      |   33 +++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/iw/advanced_storage.py b/iw/advanced_storage.py
> index 6384d5f..a21a8e7 100644
> --- a/iw/advanced_storage.py
> +++ b/iw/advanced_storage.py
> @@ -394,7 +394,7 @@ def addFcoeDrive(anaconda):
>      dialog.destroy()
>      return rc
>  
> -def addIscsiDrive(anaconda):
> +def addIscsiDrive(anaconda, bind=False):
>      """
>      Displays a series of dialogs that walk the user through discovering and
>      logging into iscsi nodes.
> @@ -409,6 +409,15 @@ def addIscsiDrive(anaconda):
>              return gtk.RESPONSE_CANCEL
>          urlgrabber.grabber.reset_curl_obj()
>  
> +    # This will modify behaviour of iscsi.discovery() function
> +    if storage.iscsi.iscsi().mode == "none" and not bind:
> +        storage.iscsi.iscsi().delete_interfaces()
> +    elif (storage.iscsi.iscsi().mode == "none" and bind
> +          or storage.iscsi.iscsi().mode == "bind"):

I'm not liking how this reads.  I can read it as:

    elif (storage.iscsi.iscsi().mode == "none" and bind) or \
         storage.iscsi.iscsi().mode == "bind"

or:

    elif storage.iscsi.iscsi().mode == "none" and \
         (storage.iscsi.iscsi().mode == "bind" or bind)

I say it needs better parens for readability.

> +            active = set(network.getActiveNetDevs())
> +            created = set(storage.iscsi.iscsi().ifaces.values())
> +            storage.iscsi.iscsi().create_interfaces(active - created)
> +
>      wizard = iSCSIGuiWizard()
>      login_ok_nodes = pih.drive_iscsi_addition(anaconda, wizard)
>      if len(login_ok_nodes):
> @@ -456,6 +465,10 @@ def addDrive(anaconda):
>      if not storage.iscsi.has_iscsi():
>          dxml.get_widget("iscsiRadio").set_sensitive(False)
>          dxml.get_widget("iscsiRadio").set_active(False)
> +        dxml.get_widget("iscsiBindCheck").set_sensitive(False)
> +    else:
> +        dxml.get_widget("iscsiBindCheck").set_active(bool(storage.iscsi.iscsi().ifaces))
> +        dxml.get_widget("iscsiBindCheck").set_sensitive(storage.iscsi.iscsi().mode == "none")
>  
>      if not storage.fcoe.has_fcoe():
>          dxml.get_widget("fcoeRadio").set_sensitive(False)
> @@ -476,7 +489,8 @@ def addDrive(anaconda):
>          return False
>  
>      if dxml.get_widget("iscsiRadio").get_active() and storage.iscsi.has_iscsi():
> -        rc = addIscsiDrive(anaconda)
> +        bind = dxml.get_widget("iscsiBindCheck").get_active()
> +        rc = addIscsiDrive(anaconda, bind)
>      elif dxml.get_widget("fcoeRadio").get_active() and storage.fcoe.has_fcoe():
>          rc = addFcoeDrive(anaconda)
>      elif dxml.get_widget("zfcpRadio") is not None and dxml.get_widget("zfcpRadio").get_active():
> diff --git a/storage/iscsi.py b/storage/iscsi.py
> index d44b822..f4dfee4 100644
> --- a/storage/iscsi.py
> +++ b/storage/iscsi.py
> @@ -103,6 +103,7 @@ class iscsi(object):
>          self._initiator = ""
>          self.initiatorSet = False
>          self.started = False
> +        self.ifaces = {}
>  
>          if flags.ibft:
>              try:
> @@ -142,6 +143,17 @@ class iscsi(object):
>                      itertools.chain(*self.discovered_targets.values())
>                      if logged_in] + self.ibftNodes
>  
> +    def _getMode(self):
> +        if not self.active_nodes():
> +            return "none"
> +        else:
> +            if self.ifaces:
> +                return "bind"
> +            else:
> +                return "default"

This if/else block doesn't really need to be in an else block by itself.

> +    mode = property(_getMode)
> +
>      def _mark_node_active(self, node, active=True):
>          """Mark node as one logged in to
>  
> @@ -189,6 +201,37 @@ class iscsi(object):
>          if intf:
>              w.pop()
>  
> +    def create_interfaces(self, ifaces):
> +        for iface in ifaces:
> +            iscsi_iface_name = "iface%d" % len(self.ifaces)
> +            #iscsiadm -m iface -I iface0 --op=new
> +            iutil.execWithRedirect("iscsiadm",
> +                                   ["-m", "iface", "-I", iscsi_iface_name, "--op=new"],
> +                                   stdout="/dev/tty5",
> +                                   stderr="/dev/tty5")
> +            #iscsiadm -m iface -I iface0 --op=update -n iface.net_ifacename -v eth0
> +            iutil.execWithRedirect("iscsiadm",
> +                                   ["-m", "iface", "-I", iscsi_iface_name,
> +                                    "--op=update", "-n",
> +                                    "iface.net_ifacename", "-v", iface],
> +                                   stdout="/dev/tty5",
> +                                   stderr="/dev/tty5")
> +
> +            self.ifaces[iscsi_iface_name] = iface
> +            log.debug("created_interface %s:%s" % (iscsi_iface_name, iface))
> +
> +    def delete_interfaces(self):
> +        if not self.ifaces:
> +            return None
> +        for iscsi_iface_name in self.ifaces:
> +            #iscsiadm -m iface -I iface0 --op=delete
> +            iutil.execWithRedirect("iscsiadm",
> +                                   ["-m", "iface", "-I", iscsi_iface_name,
> +                                    "--op=delete"],
> +                                   stdout="/dev/tty5",
> +                                   stderr="/dev/tty5")
> +        self.ifaces = {}
> +
>      def startup(self, intf = None):
>          if self.started:
>              return
> diff --git a/ui/adddrive.glade b/ui/adddrive.glade
> index a085176..5440966 100644
> --- a/ui/adddrive.glade
> +++ b/ui/adddrive.glade
> @@ -184,6 +184,39 @@
>  	      </child>
>  
>  	      <child>
> +		<widget class="GtkAlignment" id="alignment2">
> +		  <property name="visible">True</property>
> +		  <property name="xalign">0.5</property>
> +		  <property name="yalign">0.5</property>
> +		  <property name="xscale">1</property>
> +		  <property name="yscale">1</property>
> +		  <property name="top_padding">0</property>
> +		  <property name="bottom_padding">0</property>
> +		  <property name="left_padding">18</property>
> +		  <property name="right_padding">0</property>
> +
> +		  <child>
> +		    <widget class="GtkCheckButton" id="iscsiBindCheck">
> +		      <property name="visible">True</property>
> +		      <property name="can_focus">True</property>
> +		      <property name="label" translatable="yes">_Bind targets to network interfaces</property>
> +		      <property name="use_underline">True</property>
> +		      <property name="relief">GTK_RELIEF_NORMAL</property>
> +		      <property name="focus_on_click">True</property>
> +		      <property name="active">False</property>
> +		      <property name="inconsistent">False</property>
> +		      <property name="draw_indicator">True</property>
> +		    </widget>
> +		  </child>
> +		</widget>
> +		<packing>
> +		  <property name="padding">0</property>
> +		  <property name="expand">False</property>
> +		  <property name="fill">False</property>
> +		</packing>
> +	      </child>
> +
> +	      <child>
>  		<widget class="GtkRadioButton" id="zfcpRadio">
>  		  <property name="visible">True</property>
>  		  <property name="label" translatable="yes">Add _ZFCP LUN</property>
> -- 
> 1.7.4
> 
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list redhat com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

-- 
David Cantrell <dcantrell redhat com>
Supervisor, Installer Engineering Team
Red Hat, Inc. | Westford, MA | EST5EDT


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]