[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