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

Vratislav Podzimek vpodzime at redhat.com
Thu Nov 28 08:49:01 UTC 2013


On Wed, 2013-11-27 at 14:54 -0500, Samantha N. Bueno wrote:
> 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.
That's what I meant. There is no point in catching and re-raising the
exception without any other actions. You can just do the actions outside
of the try-except block with the same result.

>  
> > > +    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.
If you call self._check_discover decorated with the @gtk_action_nowait
at the end of self._discover, it should do the same as what you have
just wihout the polling -- wait for the discover to be finished and set
the UI elements accordingly (safely) in the Gtk main loop. If not, there
is a serious bug in code we use in multiple other places and we should
fix it.

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list