[PATCH 1/3] Add support for adding zFCP devices in the GUI. (#994423)

Samantha N. Bueno sbueno+anaconda at redhat.com
Wed Nov 27 19:54:45 UTC 2013


On Wed, Nov 27, 2013 at 10:14:20AM +0100, Vratislav Podzimek wrote:
> On Tue, 2013-11-26 at 14:41 -0500, Samantha N. Bueno wrote:
> > This gives s390x users the ability to add a zFCP device for use during
> > installation. Even if a device is "offline", if the correct credentials
> > are entered, the device will be detected and turned "online" for use.
> > 
> > Resolves: rhbz#994423
> > ---
> >  pyanaconda/constants.py                        |   1 +
> >  pyanaconda/ui/gui/spokes/advstorage/zfcp.glade | 447 +++++++++++++++++++++++++
> >  pyanaconda/ui/gui/spokes/advstorage/zfcp.py    | 189 +++++++++++
> >  3 files changed, 637 insertions(+)
> >  create mode 100644 pyanaconda/ui/gui/spokes/advstorage/zfcp.glade
> >  create mode 100644 pyanaconda/ui/gui/spokes/advstorage/zfcp.py
> 
> > diff --git a/pyanaconda/ui/gui/spokes/advstorage/zfcp.py b/pyanaconda/ui/gui/spokes/advstorage/zfcp.py
> > new file mode 100644
> > index 0000000..5b55ec7
> > --- /dev/null
> > +++ b/pyanaconda/ui/gui/spokes/advstorage/zfcp.py
> > <>
> > +
> > +    def run(self):
> > +        rc = self.window.run()
> > +        self.window.destroy()
> > +        # We need to call this to get the device nodes to show up
> > +        # in our devicetree.
> > +        if self._update_devicetree:
> > +            self.storage.devicetree.populate()
> > +        return rc
> > +
> > +    def _set_configure_sensitive(self, sensitivity):
> > +        for child in self._configureGrid.get_children():
> > +            if child == self._deviceEntry:
> > +                self._deviceEntry.set_sensitive(True)
> Could there be a case where the self._deviceEntry won't be
> self._configureGrid's child? If not, it could be set sensitive outside
> of the loop.

I don't think so, so I'll add that edit.
 
> > +            elif child != self._conditionNotebook:
> > +                child.set_sensitive(sensitivity)
> > +
> > +    def _device_valid(self, dev):
> > +        """ Check whether the entered CCW entry is valid. """
> > +        widget = self.builder.get_object("deviceEntry")
> All objects should be pulled only once, in the refresh method (or in a
> separate initialize method).

Good call; fixed locally.
 
> > +        text = widget.get_text()
> > +
> > +        try:
> > +            device = dev.sanitizeDeviceInput(text)
> > +            return device
> > +        except ValueError as e:
> > +            raise ValueError(e)
> This except block does nothing and so do the other similar except block.

I'm not sure I follow. This raises an error message if the user enters a
string that does not match what is expected to be considered valid
input. In _on_start_clicked, the ValueError is caught and the error
string displayed on screen.
I do agree that it's ugly/awkward to just re-raise the very same error
which was just caught though. I can re-factor these blobs.
 
> > +    def on_start_clicked(self, *args):
> > +        """ Go through the process of validating entry contents and then
> > +            attempt to add the device.
> > +        """
> > +        # First update widgets
> > +        self._startButton.hide()
> > +        self._cancelButton.set_sensitive(False)
> > +        self._okButton.set_sensitive(False)
> > +
> > +        self._conditionNotebook.set_current_page(1)
> > +        self._set_configure_sensitive(False)
> > +        self._deviceEntry.set_sensitive(False)
> > +
> > +        # Make a zFCP object with some dummy credentials so we can validate our
> > +        # actual input
> > +        self._conditionNotebook.set_current_page(1)
> > +        dev = ZFCPDevice("0.0.0000", "0x0000000000000000", "0x0000000000000000")
> > +        # below really, really is ugly and needs to be re-factored, but this
> > +        # should give a good base idea as far as expected behavior should go
> > +        try:
> > +            device = self._device_valid(dev)
> > +            wwpn = self._wwpn_valid(dev)
> > +            lun = self._lun_valid(dev)
> > +        except ValueError as e:
> > +            _config_error = str(e)
> > +            self.builder.get_object("deviceErrorLabel").set_text(_config_error)
> > +            self._conditionNotebook.set_current_page(2)
> > +
> > +        spinner = self.builder.get_object("waitSpinner")
> > +        spinner.start()
> > +
> > +        threadMgr.add(AnacondaThread(name=constants.THREAD_ZFCP_DISCOVER, target=self._discover, args=(device, wwpn, lun)))
> > +        GLib.timeout_add(30, self._check_discover)
> Is there any reason for not calling self._check_discover at the end of
> the self._discover method instead of doing this 30ms polling?
> self._check_discover would just need to be decorated with the
> @gtk_action_nowait decorator so that it is run in the main thread.
 
I thought it was prudent to add the poll since it can take some time for
the devices to be discovered and switched to an "online" state.

Adding the @gtk_action_nowait decorator seems to leave the installer
stuck in an infinite loop.

> > +    def _discover(self, *args):
> > +        """ Given the configuration options from a user, attempt to discover
> > +            a zFCP device. This includes searching black-listed devices.
> > +        """
> > +        # attempt to add the device
> > +        try:
> > +            self.zfcp.addFCP(args[0], args[1], args[2])
> > +            self._update_devicetree = True
> > +        except ValueError as e:
> > +            self._discoveryError = str(e)
> > +            return
> > +
> > +        if len(self._discoveredNodes) == 0:
> self._discoveredNodes doesn't seem to be updated anywhere.

That is correct; removed. :)
 
> > +            self._discoveryErorr = "No devices discovered."
>                               ^^typo
> 
> -- 
> Vratislav Podzimek
> 
> Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches


More information about the anaconda-patches mailing list