This includes fixes suggested from the first review. The flow of this is also a bit different from when I first posted as well, thanks to discussions with dlehman and mizmo (read: simpler and less clunky). The text install is basically the same, but for an updated screencast of the graphical version here's a link: http://sbueno.fedorapeople.org/1001070-graphical-25feb.webm
jstodola mentioned the "zerombr" kickstart command last time; support for that is here; if "zerombr" is in a ks file and unformatted DASDs are specified, they automatically have dasdfmt run against them.
A few points that are sort of minor but which bug me, all graphical: * The link on the dialog box back to the summary hub doesn't work, so I sort of force it myself in what's probably a less correct way. I'm not sure what format the URI should be, so if anyone knows, let me know.
* Again following a return to the summary hub, while letting dasdfmt run in the background, the UI sort of freezes up; I'm guessing I need some sort of decorator somewhere, maybe?
* I think my thread handling is jumbled and awkward in places (GUI storage), then again trying to cleanly work in the "zerombr" command was no picnic. I'd be greatly in favor of only allowing "zerombr" as valid if in text mode; the graphical code is hideous IMO.
This gets rid of the DASD class and instead introduces a small number of functions to interface with and manage DASDs. This is completely divorced from the anaconda code and much more lightweight.
(The corresponding Fedora bug for this is 859997.)
Resolves:rhbz#1001070
--- blivet/__init__.py | 13 ++- blivet/dasd.py | 222 ---------------------------------------------- blivet/devicelibs/dasd.py | 134 ++++++++++++++++++++++++++++ blivet/devicetree.py | 6 +- 4 files changed, 143 insertions(+), 232 deletions(-) delete mode 100644 blivet/dasd.py create mode 100644 blivet/devicelibs/dasd.py
diff --git a/blivet/__init__.py b/blivet/__init__.py index 6908b2a..037a40f 100644 --- a/blivet/__init__.py +++ b/blivet/__init__.py @@ -77,11 +77,11 @@ import devicefactory from devicelibs.dm import name_from_dm_node from devicelibs.crypto import generateBackupPassphrase from devicelibs.edd import get_edd_dict +from devicelibs.dasd import * from udev import udev_trigger import iscsi import fcoe import zfcp -import dasd import util import arch from flags import flags @@ -283,6 +283,7 @@ class Blivet(object): self.encryptionRetrofit = False self.autoPartitionRequests = [] self.eddDict = {} + self.dasd = []
self.__luksDevs = {} self.size_sets = [] @@ -291,7 +292,6 @@ class Blivet(object): self.iscsi = iscsi.iscsi() self.fcoe = fcoe.fcoe() self.zfcp = zfcp.ZFCP() - self.dasd = dasd.DASD()
self._nextID = 0 self._dumpFile = "%s/storage.state" % tempfile.gettempdir() @@ -399,12 +399,11 @@ class Blivet(object): self.iscsi.startup() self.fcoe.startup() self.zfcp.startup() - self.dasd.startup(None, - self.config.exclusiveDisks, - self.config.initializeDisks) + self.dasd = make_dasd_list(self.dasd, self.devicetree) + if self.dasd: # Reset the internal dasd list (823534) - self.dasd.clear_device_list() + clear_dasd_list(self.dasd)
self.devicetree.reset(conf=self.config, passphrase=self.encryptionPassphrase, @@ -1672,7 +1671,7 @@ class Blivet(object): self.iscsi.write(ROOT_PATH, self) self.fcoe.write(ROOT_PATH) self.zfcp.write(ROOT_PATH) - self.dasd.write(ROOT_PATH) + write_dasd_conf(self.dasd, ROOT_PATH)
def turnOnSwap(self, upgrading=None): self.fsset.turnOnSwap(rootPath=ROOT_PATH, diff --git a/blivet/dasd.py b/blivet/dasd.py deleted file mode 100644 index c2ec2e4..0000000 --- a/blivet/dasd.py +++ /dev/null @@ -1,222 +0,0 @@ -# -# dasd.py - DASD class -# -# Copyright (C) 2009, 2010 Red Hat, Inc. All rights reserved. -# -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 2 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program. If not, see http://www.gnu.org/licenses/. -# -# Red Hat Author(s): David Cantrell dcantrell@redhat.com -# - -import sys -import os -from .errors import DasdFormatError -from .devices import deviceNameToDiskByPath -from . import util -from . import arch -from .udev import udev_trigger - -import logging -log = logging.getLogger("blivet") - -import gettext -_ = lambda x: gettext.ldgettext("blivet", x) -P_ = lambda x, y, z: gettext.ldngettext("blivet", x, y, z) - -def getDasdPorts(): - """ Return comma delimited string of valid DASD ports. """ - ports = [] - - f = open("/proc/dasd/devices", "r") - lines = map(lambda x: x.strip(), f.readlines()) - f.close() - - for line in lines: - if "unknown" in line: - continue - - if "(FBA )" in line or "(ECKD)" in line: - ports.append(line.split('(')[0]) - - return ','.join(ports) - -class DASD: - """ Controlling class for DASD interaction before the storage code in - anaconda has initialized. - - The DASD class can determine if any DASD devices on the system are - unformatted and can perform a dasdfmt on them. - """ - - def __init__(self): - self._dasdlist = [] - self._devices = [] # list of DASDDevice objects - self.totalCylinders = 0 - self._completedCylinders = 0.0 - self._maxFormatJobs = 0 - self.dasdfmt = "/sbin/dasdfmt" - self.commonArgv = ["-y", "-d", "cdl", "-b", "4096"] - self.started = False - - def __call__(self): - return self - - def startup(self, intf, exclusiveDisks, zeroMbr): - """ Look for any unformatted DASDs in the system and offer the user - the option for format them with dasdfmt or exit the installer. - """ - if self.started: - return - - self.started = True - - if not arch.isS390(): - return - - # Trigger udev data about the dasd devices on the system - udev_trigger(action="change", name="dasd*") - - log.info("Checking for unformatted DASD devices:") - - for device in os.listdir("/sys/block"): - if not device.startswith("dasd"): - continue - - statusfile = "/sys/block/%s/device/status" % (device,) - if not os.path.isfile(statusfile): - continue - - f = open(statusfile, "r") - status = f.read().strip() - f.close() - - if status in ["unformatted"] and device not in exclusiveDisks: - bypath = deviceNameToDiskByPath(device) - if not bypath: - bypath = "/dev/" + device - - log.info(" %s (%s) status is %s, needs dasdfmt" % (device, - bypath, - status,)) - self._dasdlist.append((device, bypath)) - - if not len(self._dasdlist): - log.info(" no unformatted DASD devices found") - return - - askUser = True - - if zeroMbr: - askUser = False - elif not intf and not zeroMbr: - log.info(" non-interactive kickstart install without zerombr " - "command, unable to run dasdfmt, exiting installer") - sys.exit(0) - - c = len(self._dasdlist) - - if intf and askUser: - devs = '' - for dasd, bypath in self._dasdlist: - devs += "%s\n" % (bypath,) - - rc = intf.questionInitializeDASD(c, devs) - if rc == 1: - log.info(" not running dasdfmt, continuing installation") - return - - # gather total cylinder count - argv = ["-t", "-v"] + self.commonArgv - for dasd, bypath in self._dasdlist: - buf = util.capture_output([self.dasdfmt, argv, "/dev/" + dasd]) - for line in buf.splitlines(): - if line.startswith("Drive Geometry: "): - # line will look like this: - # Drive Geometry: 3339 Cylinders * 15 Heads = 50085 Tracks - cyls = long(filter(lambda s: s, line.split(' '))[2]) - self.totalCylinders += cyls - break - - # format DASDs - argv = ["-P"] + self.commonArgv - update = self._updateProgressWindow - - title = P_("Formatting DASD Device", "Formatting DASD Devices", c) - msg = P_("Preparing %d DASD device for use with Linux..." % c, - "Preparing %d DASD devices for use with Linux..." % c, c) - - if intf: - if self.totalCylinders: - pw = intf.progressWindow(title, msg, 1.0) - else: - pw = intf.progressWindow(title, msg, 100, pulse=True) - - for dasd, bypath in self._dasdlist: - log.info("Running dasdfmt on %s" % (bypath,)) - arglist = argv + ["/dev/" + dasd] - - try: - rc = util.run_program([self.dasdfmt] + arglist) - except Exception as e: - raise DasdFormatError(e, bypath) - - if rc: - raise DasdFormatError("dasdfmt failed: %s" % rc, bypath) - - if intf: - pw.pop() - - def addDASD(self, dasd): - """ Adds a DASDDevice to the internal list of DASDs. """ - if dasd and dasd not in self._devices: - self._devices.append(dasd) - - def removeDASD(self, dasd): - """ Removes a DASDDevice from the internal list of DASDs. """ - if dasd and dasd in self._devices: - self._devices.remove(dasd) - - def clear_device_list(self): - """ Clear the device list to force re-populate on next access. """ - self._devices = [] - - def write(self, ROOT_PATH): - """ Write /etc/dasd.conf to target system for all DASD devices - configured during installation. - """ - if self._devices == []: - return - - f = open(os.path.realpath(ROOT_PATH + "/etc/dasd.conf"), "w") - for dasd in sorted(self._devices, key=lambda d: d.name): - fields = [dasd.busid] + dasd.getOpts() - f.write("%s\n" % (" ".join(fields),)) - f.close() - - def _updateProgressWindow(self, data, callback_data=None): - """ Reads progress output from dasdfmt and collects the number of - cylinders completed so the progress window can update. - """ - if not callback_data: - return - - if data == '\n': - # each newline we see in this output means one more cylinder done - self._completedCylinders += 1.0 - callback_data.set(self._completedCylinders / self.totalCylinders) - -# Create DASD singleton -DASD = DASD() - -# vim:tw=78:ts=4:et:sw=4 diff --git a/blivet/devicelibs/dasd.py b/blivet/devicelibs/dasd.py new file mode 100644 index 0000000..7b71552 --- /dev/null +++ b/blivet/devicelibs/dasd.py @@ -0,0 +1,134 @@ +# +# dasd.py - DASD functions +# +# Copyright (C) 2013 Red Hat, Inc. All rights reserved. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +# +# Red Hat Author(s): Samantha N. Bueno +# + +import os +from blivet.errors import DasdFormatError +from blivet.devices import deviceNameToDiskByPath +from blivet import util +from blivet import arch +from blivet.udev import udev_trigger + +import logging +log = logging.getLogger("blivet") + +import gettext +_ = lambda x: gettext.ldgettext("blivet", x) +P_ = lambda x, y, z: gettext.ldngettext("blivet", x, y, z) + +def get_dasd_ports(): + """ Return comma delimited string of valid DASD ports. """ + ports = [] + + with open("/proc/dasd/devices", "r") as f: + lines = (line.strip() for line in f.readlines()) + f.close() + + for line in lines: + if "unknown" in line: + continue + + if "(FBA )" in line or "(ECKD)" in line: + ports.append(line.split('(')[0]) + + return ','.join(ports) + +def format_dasd(dasd): + """ Run dasdfmt on a DASD. Aside from one type of device noted below, this + function _does not_ check if a DASD needs to be formatted, but rather, + assumes the list passed needs formatting. + + We don't need to show or update any progress bars, since disk actions + will be taking place all in the progress hub, which is just one big + progress bar. + """ + try: + rc = util.run_program(["/sbin/dasdfmt", "-y", "-d", "cdl", "-b", "4096", "/dev/" + dasd]) + except Exception as err: + raise DasdFormatError(err) + + if rc: + raise DasdFormatError("dasdfmt failed: %s" % rc) + +def clear_dasd_list(dasds): + """ Zero out the list of DASDs. """ + dasds = [] + return dasds + +def make_dasd_list(dasds, devicetree): + """ Create a list of DASDs recognized by the system. """ + if not arch.isS390(): + return + + # Trigger udev data about the dasd devices on the system + udev_trigger(action="change", name="dasd*") + log.info("Generating DASD list...") + for dev in devicetree.getDevicesByType("dasd"): + if dev not in dasds: + dasds.append(dev) + + return dasds + +def make_unformatted_dasd_list(dasds): + """ Return a list of DASDS which are not formatted. """ + unformatted = [] + + for dasd in dasds: + if dasd_needs_format(dasd): + unformatted.append(dasd) + + return unformatted + +def dasd_needs_format(dasd): + """ Check if a DASD needs to have dasdfmt run against it or not. + Return True if we do need dasdfmt, False if not. + """ + statusfile = "/sys/block/%s/device/status" % (dasd,) + if not os.path.isfile(statusfile): + return False + + with open(statusfile, "r") as f: + status = f.read().strip() + f.close() + + if status in ["unformatted"]: + bypath = deviceNameToDiskByPath(dasd) + if not bypath: + bypath = "/dev/" + dasd + + log.info(" %s (%s) status is %s, needs dasdfmt" % (dasd, bypath, + status,)) + return True + + return False + + +def write_dasd_conf(disks, ROOT_PATH): + """ Write /etc/dasd.conf to target system for all DASD devices + configured during installation. + """ + if disks == {}: + return + + with open(os.path.realpath(ROOT_PATH + "/etc/dasd.conf"), "w") as f: + for dasd in sorted(disks, key=lambda d: d.name): + fields = [dasd.busid] + dasd.getOpts() + f.write("%s\n" % (" ".join(fields),)) + f.close() diff --git a/blivet/devicetree.py b/blivet/devicetree.py index 8a79f14..fa28898 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -946,7 +946,7 @@ class DeviceTree(object): info["ID_FS_TYPE"] = "multipath_member"
if diskType == DASDDevice: - self.dasd.addDASD(device) + self.dasd.append(device)
self._addDevice(device) return device @@ -1830,7 +1830,7 @@ class DeviceTree(object): lvm.lvm_cc_addFilterRejectRegexp(device.name)
if isinstance(device, DASDDevice): - self.dasd.removeDASD(device) + self.dasd.remove(device)
def unhide(self, device): # the hidden list should be in leaves-first order @@ -1846,7 +1846,7 @@ class DeviceTree(object): parent.addChild()
if isinstance(device, DASDDevice): - self.dasd.addDASD(device) + self.dasd.append(device)
def setupDiskImages(self): """ Set up devices to represent the disk image files. """
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
This gets rid of the DASD class and instead introduces a small number of functions to interface with and manage DASDs. This is completely divorced from the anaconda code and much more lightweight.
(The corresponding Fedora bug for this is 859997.)
Resolves:rhbz#1001070
blivet/__init__.py | 13 ++- blivet/dasd.py | 222 ---------------------------------------------- blivet/devicelibs/dasd.py | 134 ++++++++++++++++++++++++++++ blivet/devicetree.py | 6 +- 4 files changed, 143 insertions(+), 232 deletions(-) delete mode 100644 blivet/dasd.py create mode 100644 blivet/devicelibs/dasd.py
diff --git a/blivet/devicelibs/dasd.py b/blivet/devicelibs/dasd.py new file mode 100644 index 0000000..7b71552 --- /dev/null +++ b/blivet/devicelibs/dasd.py @@ -0,0 +1,134 @@ +# +# dasd.py - DASD functions +# +# Copyright (C) 2013 Red Hat, Inc. All rights reserved. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +# +# Red Hat Author(s): Samantha N. Bueno +#
+import os +from blivet.errors import DasdFormatError +from blivet.devices import deviceNameToDiskByPath +from blivet import util +from blivet import arch +from blivet.udev import udev_trigger
+import logging +log = logging.getLogger("blivet")
+import gettext +_ = lambda x: gettext.ldgettext("blivet", x) +P_ = lambda x, y, z: gettext.ldngettext("blivet", x, y, z)
+def get_dasd_ports():
- """ Return comma delimited string of valid DASD ports. """
- ports = []
- with open("/proc/dasd/devices", "r") as f:
lines = (line.strip() for line in f.readlines())f.close()
The with block closes the file for you. And I believe you need to close the file after reading lines not after creating the generator. So you should drop the f.close() line and indent the for loop more to become a part of the with block.
- for line in lines:
if "unknown" in line:continueif "(FBA )" in line or "(ECKD)" in line:ports.append(line.split('(')[0])- return ','.join(ports)
+def format_dasd(dasd):
- """ Run dasdfmt on a DASD. Aside from one type of device noted below, this
function _does not_ check if a DASD needs to be formatted, but rather,assumes the list passed needs formatting.We don't need to show or update any progress bars, since disk actionswill be taking place all in the progress hub, which is just one bigprogress bar.- """
- try:
rc = util.run_program(["/sbin/dasdfmt", "-y", "-d", "cdl", "-b", "4096", "/dev/" + dasd])- except Exception as err:
raise DasdFormatError(err)- if rc:
raise DasdFormatError("dasdfmt failed: %s" % rc)+def clear_dasd_list(dasds):
- """ Zero out the list of DASDs. """
- dasds = []
- return dasds
Same question as with the previous version of the patch -- do you want to clear the passed list or just return a new empty one?
+def make_dasd_list(dasds, devicetree):
- """ Create a list of DASDs recognized by the system. """
- if not arch.isS390():
return- # Trigger udev data about the dasd devices on the system
- udev_trigger(action="change", name="dasd*")
- log.info("Generating DASD list...")
- for dev in devicetree.getDevicesByType("dasd"):
if dev not in dasds:dasds.append(dev)- return dasds
+def make_unformatted_dasd_list(dasds):
- """ Return a list of DASDS which are not formatted. """
- unformatted = []
- for dasd in dasds:
if dasd_needs_format(dasd):unformatted.append(dasd)- return unformatted
+def dasd_needs_format(dasd):
- """ Check if a DASD needs to have dasdfmt run against it or not.
Return True if we do need dasdfmt, False if not.- """
- statusfile = "/sys/block/%s/device/status" % (dasd,)
- if not os.path.isfile(statusfile):
return False- with open(statusfile, "r") as f:
status = f.read().strip()f.close()
No need to close the file explicitly in the with block. The same applies in one more place few lines below.
On Tue, Feb 25, 2014 at 12:54:13PM +0100, Vratislav Podzimek wrote:
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
This gets rid of the DASD class and instead introduces a small number of functions to interface with and manage DASDs. This is completely divorced from the anaconda code and much more lightweight.
(The corresponding Fedora bug for this is 859997.)
Resolves:rhbz#1001070
blivet/__init__.py | 13 ++- blivet/dasd.py | 222 ---------------------------------------------- blivet/devicelibs/dasd.py | 134 ++++++++++++++++++++++++++++ blivet/devicetree.py | 6 +- 4 files changed, 143 insertions(+), 232 deletions(-) delete mode 100644 blivet/dasd.py create mode 100644 blivet/devicelibs/dasd.py
diff --git a/blivet/devicelibs/dasd.py b/blivet/devicelibs/dasd.py new file mode 100644 index 0000000..7b71552 --- /dev/null +++ b/blivet/devicelibs/dasd.py @@ -0,0 +1,134 @@ +# +# dasd.py - DASD functions +# +# Copyright (C) 2013 Red Hat, Inc. All rights reserved. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +# +# Red Hat Author(s): Samantha N. Bueno +#
+import os +from blivet.errors import DasdFormatError +from blivet.devices import deviceNameToDiskByPath +from blivet import util +from blivet import arch +from blivet.udev import udev_trigger
+import logging +log = logging.getLogger("blivet")
+import gettext +_ = lambda x: gettext.ldgettext("blivet", x) +P_ = lambda x, y, z: gettext.ldngettext("blivet", x, y, z)
+def get_dasd_ports():
- """ Return comma delimited string of valid DASD ports. """
- ports = []
- with open("/proc/dasd/devices", "r") as f:
lines = (line.strip() for line in f.readlines())f.close()The with block closes the file for you. And I believe you need to close the file after reading lines not after creating the generator. So you should drop the f.close() line and indent the for loop more to become a part of the with block.
Thanks, I've fixed and tested that locally and all is still well. Fixed in the other locations you've noted as well.
- for line in lines:
if "unknown" in line:continueif "(FBA )" in line or "(ECKD)" in line:ports.append(line.split('(')[0])- return ','.join(ports)
+def format_dasd(dasd):
- """ Run dasdfmt on a DASD. Aside from one type of device noted below, this
function _does not_ check if a DASD needs to be formatted, but rather,assumes the list passed needs formatting.We don't need to show or update any progress bars, since disk actionswill be taking place all in the progress hub, which is just one bigprogress bar.- """
- try:
rc = util.run_program(["/sbin/dasdfmt", "-y", "-d", "cdl", "-b", "4096", "/dev/" + dasd])- except Exception as err:
raise DasdFormatError(err)- if rc:
raise DasdFormatError("dasdfmt failed: %s" % rc)+def clear_dasd_list(dasds):
- """ Zero out the list of DASDs. """
- dasds = []
- return dasds
Same question as with the previous version of the patch -- do you want to clear the passed list or just return a new empty one?
Hm, well I guess this function isn't really necessary since, TBH it seems pointless to make a function that just returns an empty list. I'll just get rid of that and in blivet/__init__.py I will replace the function call with an empty list.
+def make_dasd_list(dasds, devicetree):
- """ Create a list of DASDs recognized by the system. """
- if not arch.isS390():
return- # Trigger udev data about the dasd devices on the system
- udev_trigger(action="change", name="dasd*")
- log.info("Generating DASD list...")
- for dev in devicetree.getDevicesByType("dasd"):
if dev not in dasds:dasds.append(dev)- return dasds
+def make_unformatted_dasd_list(dasds):
- """ Return a list of DASDS which are not formatted. """
- unformatted = []
- for dasd in dasds:
if dasd_needs_format(dasd):unformatted.append(dasd)- return unformatted
+def dasd_needs_format(dasd):
- """ Check if a DASD needs to have dasdfmt run against it or not.
Return True if we do need dasdfmt, False if not.- """
- statusfile = "/sys/block/%s/device/status" % (dasd,)
- if not os.path.isfile(statusfile):
return False- with open(statusfile, "r") as f:
status = f.read().strip()f.close()No need to close the file explicitly in the with block. The same applies in one more place few lines below.
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
This gets rid of the DASD class and instead introduces a small number of functions to interface with and manage DASDs. This is completely divorced from the anaconda code and much more lightweight.
You know I am quite happy to see this code moved into devicelibs, where it belongs, but I have a couple of comments below.
(The corresponding Fedora bug for this is 859997.)
Resolves:rhbz#1001070
blivet/__init__.py | 13 ++- blivet/dasd.py | 222 ---------------------------------------------- blivet/devicelibs/dasd.py | 134 ++++++++++++++++++++++++++++ blivet/devicetree.py | 6 +- 4 files changed, 143 insertions(+), 232 deletions(-) delete mode 100644 blivet/dasd.py create mode 100644 blivet/devicelibs/dasd.py
diff --git a/blivet/__init__.py b/blivet/__init__.py index 6908b2a..037a40f 100644 --- a/blivet/__init__.py +++ b/blivet/__init__.py @@ -77,11 +77,11 @@ import devicefactory from devicelibs.dm import name_from_dm_node from devicelibs.crypto import generateBackupPassphrase from devicelibs.edd import get_edd_dict +from devicelibs.dasd import *
This would be better as three lines that import make_dasd_list, clear_dasd_list, and write_dasd_conf individually. I know there are several 'import *' lines in there already, but let's not add more of them.
<snip>
diff --git a/blivet/devicelibs/dasd.py b/blivet/devicelibs/dasd.py new file mode 100644 index 0000000..7b71552 --- /dev/null +++ b/blivet/devicelibs/dasd.py @@ -0,0 +1,134 @@ +# +# dasd.py - DASD functions +# +# Copyright (C) 2013 Red Hat, Inc. All rights reserved. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +# +# Red Hat Author(s): Samantha N. Bueno +#
+import os +from blivet.errors import DasdFormatError +from blivet.devices import deviceNameToDiskByPath +from blivet import util +from blivet import arch +from blivet.udev import udev_trigger
+import logging +log = logging.getLogger("blivet")
+import gettext +_ = lambda x: gettext.ldgettext("blivet", x) +P_ = lambda x, y, z: gettext.ldngettext("blivet", x, y, z)
+def get_dasd_ports():
- """ Return comma delimited string of valid DASD ports. """
- ports = []
- with open("/proc/dasd/devices", "r") as f:
lines = (line.strip() for line in f.readlines())f.close()- for line in lines:
if "unknown" in line:continueif "(FBA )" in line or "(ECKD)" in line:ports.append(line.split('(')[0])- return ','.join(ports)
+def format_dasd(dasd):
- """ Run dasdfmt on a DASD. Aside from one type of device noted below, this
function _does not_ check if a DASD needs to be formatted, but rather,assumes the list passed needs formatting.We don't need to show or update any progress bars, since disk actionswill be taking place all in the progress hub, which is just one bigprogress bar.- """
- try:
rc = util.run_program(["/sbin/dasdfmt", "-y", "-d", "cdl", "-b", "4096", "/dev/" + dasd])- except Exception as err:
raise DasdFormatError(err)- if rc:
raise DasdFormatError("dasdfmt failed: %s" % rc)+def clear_dasd_list(dasds):
- """ Zero out the list of DASDs. """
- dasds = []
- return dasds
+def make_dasd_list(dasds, devicetree):
Can you make this take a list of devices instead? You could pass it 'storage.devices' or even 'storage.disks' if you didn't want to filter by type 'dasd' in the caller of this function. Most of the devicelibs code doesn't even know about StorageDevice -- let alone DeviceTree. I've tried to keep the lower-level subpackages (devicelibs, formats) from having any knowledge of the higher-level ones (devicetree, devices, deviceaction).
- """ Create a list of DASDs recognized by the system. """
- if not arch.isS390():
return- # Trigger udev data about the dasd devices on the system
- udev_trigger(action="change", name="dasd*")
This should have happened during system boot. Is there a reason it has to be done explicitly here?
- log.info("Generating DASD list...")
- for dev in devicetree.getDevicesByType("dasd"):
If you changed this function to take a list of devices you could change the above line to something like this:
for dev in (d for d in disks if d.type == "dasd"):
Looks okay otherwise.
David
On Tue, Feb 25, 2014 at 09:02:14AM -0600, David Lehman wrote:
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
This gets rid of the DASD class and instead introduces a small number of functions to interface with and manage DASDs. This is completely divorced from the anaconda code and much more lightweight.
You know I am quite happy to see this code moved into devicelibs, where it belongs, but I have a couple of comments below.
(The corresponding Fedora bug for this is 859997.)
Resolves:rhbz#1001070
blivet/__init__.py | 13 ++- blivet/dasd.py | 222 ---------------------------------------------- blivet/devicelibs/dasd.py | 134 ++++++++++++++++++++++++++++ blivet/devicetree.py | 6 +- 4 files changed, 143 insertions(+), 232 deletions(-) delete mode 100644 blivet/dasd.py create mode 100644 blivet/devicelibs/dasd.py
diff --git a/blivet/__init__.py b/blivet/__init__.py index 6908b2a..037a40f 100644 --- a/blivet/__init__.py +++ b/blivet/__init__.py @@ -77,11 +77,11 @@ import devicefactory from devicelibs.dm import name_from_dm_node from devicelibs.crypto import generateBackupPassphrase from devicelibs.edd import get_edd_dict +from devicelibs.dasd import *
This would be better as three lines that import make_dasd_list, clear_dasd_list, and write_dasd_conf individually. I know there are several 'import *' lines in there already, but let's not add more of them.
Sure thing, fixed locally.
<snip>
diff --git a/blivet/devicelibs/dasd.py b/blivet/devicelibs/dasd.py new file mode 100644 index 0000000..7b71552 --- /dev/null +++ b/blivet/devicelibs/dasd.py @@ -0,0 +1,134 @@ +# +# dasd.py - DASD functions +# +# Copyright (C) 2013 Red Hat, Inc. All rights reserved. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +# +# Red Hat Author(s): Samantha N. Bueno +#
+import os +from blivet.errors import DasdFormatError +from blivet.devices import deviceNameToDiskByPath +from blivet import util +from blivet import arch +from blivet.udev import udev_trigger
+import logging +log = logging.getLogger("blivet")
+import gettext +_ = lambda x: gettext.ldgettext("blivet", x) +P_ = lambda x, y, z: gettext.ldngettext("blivet", x, y, z)
+def get_dasd_ports():
- """ Return comma delimited string of valid DASD ports. """
- ports = []
- with open("/proc/dasd/devices", "r") as f:
lines = (line.strip() for line in f.readlines())f.close()- for line in lines:
if "unknown" in line:continueif "(FBA )" in line or "(ECKD)" in line:ports.append(line.split('(')[0])- return ','.join(ports)
+def format_dasd(dasd):
- """ Run dasdfmt on a DASD. Aside from one type of device noted below, this
function _does not_ check if a DASD needs to be formatted, but rather,assumes the list passed needs formatting.We don't need to show or update any progress bars, since disk actionswill be taking place all in the progress hub, which is just one bigprogress bar.- """
- try:
rc = util.run_program(["/sbin/dasdfmt", "-y", "-d", "cdl", "-b", "4096", "/dev/" + dasd])- except Exception as err:
raise DasdFormatError(err)- if rc:
raise DasdFormatError("dasdfmt failed: %s" % rc)+def clear_dasd_list(dasds):
- """ Zero out the list of DASDs. """
- dasds = []
- return dasds
+def make_dasd_list(dasds, devicetree):
Can you make this take a list of devices instead? You could pass it 'storage.devices' or even 'storage.disks' if you didn't want to filter by type 'dasd' in the caller of this function. Most of the devicelibs code doesn't even know about StorageDevice -- let alone DeviceTree. I've tried to keep the lower-level subpackages (devicelibs, formats) from having any knowledge of the higher-level ones (devicetree, devices, deviceaction).
That makes sense, and I will change this. Unformatted DASDs do not show up in storage.disks since they fail the mediaPresent check, but I can pass storage.devices.
- """ Create a list of DASDs recognized by the system. """
- if not arch.isS390():
return- # Trigger udev data about the dasd devices on the system
- udev_trigger(action="change", name="dasd*")
This should have happened during system boot. Is there a reason it has to be done explicitly here?
Hm, no. That was in the original dasd.py which I trimmed when making devicelibs/dasd.py. Wasn't sure if it should be left in, so I erred on the side of caution. Getting rid of it does not change any behavior, so it does not seem to be needed.
- log.info("Generating DASD list...")
- for dev in devicetree.getDevicesByType("dasd"):
If you changed this function to take a list of devices you could change the above line to something like this:
for dev in (d for d in disks if d.type == "dasd"):
Sure thing, fixed locally along with the above.
Looks okay otherwise.
David
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
A GtkDialog box which shows the progress of dasdfmt as it is run against unformatted DASDs.
Resolves: rhbz#1064423 --- po/POTFILES.in | 2 + pyanaconda/ui/gui/spokes/lib/dasdfmt.glade | 211 +++++++++++++++++++++++++++++ pyanaconda/ui/gui/spokes/lib/dasdfmt.py | 111 +++++++++++++++ 3 files changed, 324 insertions(+) create mode 100644 pyanaconda/ui/gui/spokes/lib/dasdfmt.glade create mode 100644 pyanaconda/ui/gui/spokes/lib/dasdfmt.py
diff --git a/po/POTFILES.in b/po/POTFILES.in index 3d53ce4..4b341fc 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -76,6 +76,7 @@ pyanaconda/ui/gui/spokes/user.py pyanaconda/ui/gui/spokes/welcome.py pyanaconda/ui/gui/spokes/lib/accordion.py pyanaconda/ui/gui/spokes/lib/cart.py +pyanaconda/ui/gui/spokes/lib/dasdfmt.py pyanaconda/ui/gui/spokes/lib/passphrase.py pyanaconda/ui/gui/spokes/lib/refresh.py pyanaconda/ui/gui/spokes/lib/resize.py @@ -99,6 +100,7 @@ pyanaconda/ui/gui/spokes/advstorage/fcoe.glade pyanaconda/ui/gui/spokes/advstorage/iscsi.glade pyanaconda/ui/gui/spokes/advstorage/zfcp.glade pyanaconda/ui/gui/spokes/lib/cart.glade +pyanaconda/ui/gui/spokes/lib/dasdfmt.glade pyanaconda/ui/gui/spokes/lib/detailederror.glade pyanaconda/ui/gui/spokes/lib/passphrase.glade pyanaconda/ui/gui/spokes/lib/refresh.glade diff --git a/pyanaconda/ui/gui/spokes/lib/dasdfmt.glade b/pyanaconda/ui/gui/spokes/lib/dasdfmt.glade new file mode 100644 index 0000000..2fdb0c9 --- /dev/null +++ b/pyanaconda/ui/gui/spokes/lib/dasdfmt.glade @@ -0,0 +1,211 @@ +<?xml version="1.0" encoding="UTF-8"?> +<interface> + <!-- interface-requires gtk+ 3.6 --> + <object class="GtkDialog" id="unformattedDasdDialog"> + <property name="can_focus">False</property> + <property name="border_width">5</property> + <property name="title" translatable="yes">UNFORMATTED DASDS</property> + <property name="modal">True</property> + <property name="type_hint">dialog</property> + <property name="decorated">False</property> + <signal name="show" handler="dasdfmt" swapped="no"/> + <child internal-child="vbox"> + <object class="GtkBox" id="dialog-vbox1"> + <property name="can_focus">False</property> + <property name="orientation">vertical</property> + <property name="spacing">2</property> + <child> + <object class="GtkLabel" id="unformattedDasdsLabel"> + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="label" translatable="yes">Formatting DASDs</property> + <attributes> + <attribute name="weight" value="bold"/> + </attributes> + </object> + <packing> + <property name="expand">False</property> + <property name="fill">True</property> + <property name="position">0</property> + </packing> + </child> + <child> + <object class="GtkNotebook" id="formatNotebook"> + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="show_tabs">False</property> + <child> + <placeholder/> + </child> + <child type="tab"> + <placeholder/> + </child> + <child> + <object class="GtkGrid" id="formatRunning"> + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="halign">center</property> + <property name="valign">center</property> + <property name="row_spacing">18</property> + <property name="column_spacing">6</property> + <child> + <object class="GtkSpinner" id="spinner4"> + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="active">True</property> + </object> + <packing> + <property name="left_attach">0</property> + <property name="top_attach">0</property> + <property name="width">1</property> + <property name="height">1</property> + </packing> + </child> + <child> + <object class="GtkLabel" id="udLabel"> + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="label">Inform user which DASD is being formatted here.</property> + </object> + <packing> + <property name="left_attach">1</property> + <property name="top_attach">0</property> + <property name="width">1</property> + <property name="height">1</property> + </packing> + </child> + <child> + <object class="GtkLabel" id="returnToHubLabel1"> + <property name="visible">True</property> + <property name="can_focus">True</property> + <property name="label" translatable="yes">You may <a href="">go back to the main menu</a> to complete other +installation options while this operation completes.</property> + <property name="use_markup">True</property> + <property name="wrap">True</property> + <property name="track_visited_links">False</property> + <signal name="activate-link" handler="return_to_hub_link_clicked" swapped="no"/> + </object> + <packing> + <property name="left_attach">0</property> + <property name="top_attach">1</property> + <property name="width">2</property> + <property name="height">1</property> + </packing> + </child> + </object> + <packing> + <property name="position">1</property> + </packing> + </child> + <child type="tab"> + <placeholder/> + </child> + <child> + <object class="GtkGrid" id="formatComplete"> + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="halign">center</property> + <property name="valign">center</property> + <property name="row_spacing">6</property> + <property name="column_spacing">6</property> + <child> + <object class="GtkImage" id="image3"> + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="stock">gtk-apply</property> + </object> + <packing> + <property name="left_attach">0</property> + <property name="top_attach">0</property> + <property name="width">1</property> + <property name="height">1</property> + </packing> + </child> + <child> + <object class="GtkLabel" id="label19"> + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="xalign">0</property> + <property name="label" translatable="yes">Disk formatting complete.</property> + <property name="wrap">True</property> + <attributes> + <attribute name="weight" value="bold"/> + </attributes> + </object> + <packing> + <property name="left_attach">1</property> + <property name="top_attach">0</property> + <property name="width">1</property> + <property name="height">1</property> + </packing> + </child> + </object> + <packing> + <property name="position">2</property> + </packing> + </child> + <child type="tab"> + <placeholder/> + </child> + <child> + <placeholder/> + </child> + <child type="tab"> + <placeholder/> + </child> + </object> + <packing> + <property name="expand">True</property> + <property name="fill">True</property> + <property name="position">3</property> + </packing> + </child> + <child internal-child="action_area"> + <object class="GtkButtonBox" id="dialog-action_area1"> + <property name="can_focus">False</property> + <property name="layout_style">end</property> + <child> + <object class="GtkButton" id="udCancelButton"> + <property name="label" translatable="yes">_Cancel</property> + <property name="visible">True</property> + <property name="can_focus">True</property> + <property name="receives_default">True</property> + <property name="use_underline">True</property> + </object> + <packing> + <property name="expand">False</property> + <property name="fill">True</property> + <property name="position">0</property> + </packing> + </child> + <child> + <object class="GtkButton" id="udOKButton"> + <property name="label" translatable="yes">_OK</property> + <property name="visible">True</property> + <property name="sensitive">False</property> + <property name="can_focus">True</property> + <property name="receives_default">True</property> + <property name="use_underline">True</property> + </object> + <packing> + <property name="expand">False</property> + <property name="fill">True</property> + <property name="position">1</property> + </packing> + </child> + </object> + <packing> + <property name="expand">False</property> + <property name="fill">True</property> + <property name="pack_type">end</property> + <property name="position">4</property> + </packing> + </child> + </object> + </child> + <action-widgets> + <action-widget response="0">udCancelButton</action-widget> + <action-widget response="1">udOKButton</action-widget> + </action-widgets> + </object> +</interface> diff --git a/pyanaconda/ui/gui/spokes/lib/dasdfmt.py b/pyanaconda/ui/gui/spokes/lib/dasdfmt.py new file mode 100644 index 0000000..802855c --- /dev/null +++ b/pyanaconda/ui/gui/spokes/lib/dasdfmt.py @@ -0,0 +1,111 @@ +# DASD format dialog +# +# Copyright (C) 2014 Red Hat, Inc. +# +# This copyrighted material is made available to anyone wishing to use, +# modify, copy, or redistribute it subject to the terms and conditions of +# the GNU General Public License v.2, or (at your option) any later version. +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY expressed or implied, including the implied warranties of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General +# Public License for more details. You should have received a copy of the +# GNU General Public License along with this program; if not, write to the +# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA +# 02110-1301, USA. Any Red Hat trademarks that are incorporated in the +# source code or documentation are not subject to the GNU General Public +# License and may only be used or replicated with the express permission of +# Red Hat, Inc. +# +# Red Hat Author(s): Samantha N. Bueno sbueno@redhat.com +# Chris Lumens clumens@redhat.com +# + +from pyanaconda.threads import threadMgr, AnacondaThread +from pyanaconda.ui.communication import hubQ +from pyanaconda.ui.gui import GUIObject +from pyanaconda.ui.gui.utils import gtk_call_once +from pyanaconda import constants +from pyanaconda.i18n import _ + +from blivet import storageInitialize +from blivet.devicelibs.dasd import format_dasd +from blivet.errors import DasdFormatError + +import logging +log = logging.getLogger("anaconda") + +__all__ = ["DasdFormatDialog"] + +class DasdFormatDialog(GUIObject): + builderObjects = ["unformattedDasdDialog"] + mainWidgetName = "unformattedDasdDialog" + uiFile = "spokes/lib/dasdfmt.glade" + + def __init__(self, data, storage, to_format): + GUIObject.__init__(self, data) + + self.storage = storage + self.to_format = to_format + + self._notebook = self.builder.get_object("formatNotebook") + self._cancel_button = self.builder.get_object("udCancelButton") + self._ok_button = self.builder.get_object("udOKButton") + self._label = self.builder.get_object("udLabel") + + def run(self): + rc = self.window.run() + self.window.destroy() + return rc + + def run_dasdfmt(self, *args): + """ + Loop through our disks and run dasdfmt against them. After that loop has + finished, launch stage two of this operation (finish_dasdfmt). + """ + for disk in self.to_format: + try: + self._label.set_text(_("Formatting /dev/%s. This may take a moment." % disk)) + format_dasd(disk) + except DasdFormatError as err: + # Log errors if formatting fails, but don't halt the installer + log.error(str(err)) + continue + + gtk_call_once(self.finish_dasdfmt) + + def dasdfmt(self, *args): + """ + This is the handler that gets called from the GtkDialog and sets things + in motion. We launch the *actual* call to run dasdfmt from a thread. + """ + # Run through all of the formatting + self._cancel_button.set_sensitive(True) + self._ok_button.set_sensitive(False) + self._notebook.set_current_page(0) + + # Loop through all of our unformatted DASDs and format them + threadMgr.add(AnacondaThread(name=constants.THREAD_DASDFMT, + target=self.run_dasdfmt, args=(self,))) + + def finish_dasdfmt(self): + """ + This is the second stage of the dasdfmt operation; now that formatting + is complete, we need to reinitialize storage so that the newly + formatted devices are added properly to the devicetree. + """ + protectedNames = map(lambda d: d.name, self.storage.protectedDevices) + threadMgr.add(AnacondaThread(name=constants.THREAD_STORAGE, target=storageInitialize, + args=(self.storage, self.data, protectedNames))) + + threadMgr.wait(constants.THREAD_STORAGE) + self._notebook.set_current_page(1) + self._ok_button.set_sensitive(True) + + def return_to_hub_link_clicked(self, label, uri): + """ + The user clicked on the link that takes them back to the hub. We need + to emit a special response ID indicating the user did not press OK. + + NOTE: There is no button with response_id=2. + """ + self.window.response(2)
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
A GtkDialog box which shows the progress of dasdfmt as it is run against unformatted DASDs.
Resolves: rhbz#1064423
po/POTFILES.in | 2 + pyanaconda/ui/gui/spokes/lib/dasdfmt.glade | 211 +++++++++++++++++++++++++++++ pyanaconda/ui/gui/spokes/lib/dasdfmt.py | 111 +++++++++++++++ 3 files changed, 324 insertions(+) create mode 100644 pyanaconda/ui/gui/spokes/lib/dasdfmt.glade create mode 100644 pyanaconda/ui/gui/spokes/lib/dasdfmt.py
diff --git a/pyanaconda/ui/gui/spokes/lib/dasdfmt.py b/pyanaconda/ui/gui/spokes/lib/dasdfmt.py new file mode 100644 index 0000000..802855c --- /dev/null +++ b/pyanaconda/ui/gui/spokes/lib/dasdfmt.py @@ -0,0 +1,111 @@ +# DASD format dialog +# +# Copyright (C) 2014 Red Hat, Inc. +# +# This copyrighted material is made available to anyone wishing to use, +# modify, copy, or redistribute it subject to the terms and conditions of +# the GNU General Public License v.2, or (at your option) any later version. +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY expressed or implied, including the implied warranties of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General +# Public License for more details. You should have received a copy of the +# GNU General Public License along with this program; if not, write to the +# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA +# 02110-1301, USA. Any Red Hat trademarks that are incorporated in the +# source code or documentation are not subject to the GNU General Public +# License and may only be used or replicated with the express permission of +# Red Hat, Inc. +# +# Red Hat Author(s): Samantha N. Bueno sbueno@redhat.com +# Chris Lumens clumens@redhat.com +#
+from pyanaconda.threads import threadMgr, AnacondaThread +from pyanaconda.ui.communication import hubQ +from pyanaconda.ui.gui import GUIObject +from pyanaconda.ui.gui.utils import gtk_call_once +from pyanaconda import constants +from pyanaconda.i18n import _
+from blivet import storageInitialize +from blivet.devicelibs.dasd import format_dasd +from blivet.errors import DasdFormatError
+import logging +log = logging.getLogger("anaconda")
+__all__ = ["DasdFormatDialog"]
+class DasdFormatDialog(GUIObject):
- builderObjects = ["unformattedDasdDialog"]
- mainWidgetName = "unformattedDasdDialog"
- uiFile = "spokes/lib/dasdfmt.glade"
- def __init__(self, data, storage, to_format):
GUIObject.__init__(self, data)self.storage = storageself.to_format = to_formatself._notebook = self.builder.get_object("formatNotebook")self._cancel_button = self.builder.get_object("udCancelButton")self._ok_button = self.builder.get_object("udOKButton")self._label = self.builder.get_object("udLabel")- def run(self):
rc = self.window.run()self.window.destroy()return rc- def run_dasdfmt(self, *args):
"""Loop through our disks and run dasdfmt against them. After that loop hasfinished, launch stage two of this operation (finish_dasdfmt)."""for disk in self.to_format:try:self._label.set_text(_("Formatting /dev/%s. This may take a moment." % disk))
This shoudl be called with gtk_call_once as it is called from a non-main thread.
format_dasd(disk)except DasdFormatError as err:# Log errors if formatting fails, but don't halt the installerlog.error(str(err))continuegtk_call_once(self.finish_dasdfmt)
Should this be called even when the dialog is exited by clicking the link button? If not, have a look at the NTPConfig dialog that uses an epoch to discard outdated actions.
- def dasdfmt(self, *args):
"""This is the handler that gets called from the GtkDialog and sets thingsin motion. We launch the *actual* call to run dasdfmt from a thread."""# Run through all of the formattingself._cancel_button.set_sensitive(True)self._ok_button.set_sensitive(False)self._notebook.set_current_page(0)# Loop through all of our unformatted DASDs and format themthreadMgr.add(AnacondaThread(name=constants.THREAD_DASDFMT,target=self.run_dasdfmt, args=(self,)))- def finish_dasdfmt(self):
"""This is the second stage of the dasdfmt operation; now that formattingis complete, we need to reinitialize storage so that the newlyformatted devices are added properly to the devicetree."""protectedNames = map(lambda d: d.name, self.storage.protectedDevices)threadMgr.add(AnacondaThread(name=constants.THREAD_STORAGE, target=storageInitialize,args=(self.storage, self.data, protectedNames)))
I believe threadMgr.add returns the thread name (at least on master). So you can store it and use it in the following wait call. Might make the code a bit clearer.
threadMgr.wait(constants.THREAD_STORAGE)self._notebook.set_current_page(1)self._ok_button.set_sensitive(True)- def return_to_hub_link_clicked(self, label, uri):
"""The user clicked on the link that takes them back to the hub. We needto emit a special response ID indicating the user did not press OK.NOTE: There is no button with response_id=2."""self.window.response(2)
On Tue, Feb 25, 2014 at 01:02:17PM +0100, Vratislav Podzimek wrote:
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
A GtkDialog box which shows the progress of dasdfmt as it is run against unformatted DASDs.
Resolves: rhbz#1064423
po/POTFILES.in | 2 + pyanaconda/ui/gui/spokes/lib/dasdfmt.glade | 211 +++++++++++++++++++++++++++++ pyanaconda/ui/gui/spokes/lib/dasdfmt.py | 111 +++++++++++++++ 3 files changed, 324 insertions(+) create mode 100644 pyanaconda/ui/gui/spokes/lib/dasdfmt.glade create mode 100644 pyanaconda/ui/gui/spokes/lib/dasdfmt.py
diff --git a/pyanaconda/ui/gui/spokes/lib/dasdfmt.py b/pyanaconda/ui/gui/spokes/lib/dasdfmt.py new file mode 100644 index 0000000..802855c --- /dev/null +++ b/pyanaconda/ui/gui/spokes/lib/dasdfmt.py
<> snip <>
- def run_dasdfmt(self, *args):
"""Loop through our disks and run dasdfmt against them. After that loop hasfinished, launch stage two of this operation (finish_dasdfmt)."""for disk in self.to_format:try:self._label.set_text(_("Formatting /dev/%s. This may take a moment." % disk))This shoudl be called with gtk_call_once as it is called from a non-main thread.
Fixed locally.
format_dasd(disk)except DasdFormatError as err:# Log errors if formatting fails, but don't halt the installerlog.error(str(err))continuegtk_call_once(self.finish_dasdfmt)Should this be called even when the dialog is exited by clicking the link button? If not, have a look at the NTPConfig dialog that uses an epoch to discard outdated actions.
Ahh, nope. I will indeed look at the NTPConfig dialog -- thanks.
- def dasdfmt(self, *args):
"""This is the handler that gets called from the GtkDialog and sets thingsin motion. We launch the *actual* call to run dasdfmt from a thread."""# Run through all of the formattingself._cancel_button.set_sensitive(True)self._ok_button.set_sensitive(False)self._notebook.set_current_page(0)# Loop through all of our unformatted DASDs and format themthreadMgr.add(AnacondaThread(name=constants.THREAD_DASDFMT,target=self.run_dasdfmt, args=(self,)))- def finish_dasdfmt(self):
"""This is the second stage of the dasdfmt operation; now that formattingis complete, we need to reinitialize storage so that the newlyformatted devices are added properly to the devicetree."""protectedNames = map(lambda d: d.name, self.storage.protectedDevices)threadMgr.add(AnacondaThread(name=constants.THREAD_STORAGE, target=storageInitialize,args=(self.storage, self.data, protectedNames)))I believe threadMgr.add returns the thread name (at least on master). So you can store it and use it in the following wait call. Might make the code a bit clearer.
Gotcha; I will make that change on the master version. threadMgr.add does not return the thread name on rhel7-branch.
threadMgr.wait(constants.THREAD_STORAGE)self._notebook.set_current_page(1)self._ok_button.set_sensitive(True)- def return_to_hub_link_clicked(self, label, uri):
"""The user clicked on the link that takes them back to the hub. We needto emit a special response ID indicating the user did not press OK.NOTE: There is no button with response_id=2."""self.window.response(2)-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Since the installer can handle unformatted DASDs, all of them should be viewable and selectable from the storage spoke.
Resolves: rhbz#1064423 --- pyanaconda/ui/lib/disks.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/pyanaconda/ui/lib/disks.py b/pyanaconda/ui/lib/disks.py index 486208b..010b9af 100644 --- a/pyanaconda/ui/lib/disks.py +++ b/pyanaconda/ui/lib/disks.py @@ -53,11 +53,24 @@ def getDisks(devicetree, fake=False): if not flags.imageInstall: devices += devicetree._hidden
- disks = [d for d in devices if d.isDisk and - d.mediaPresent and - not d.format.hidden and - not (d.protected and - d.removable)] +# disks = [d for d in devices if d.isDisk and +# d.mediaPresent and +# not d.format.hidden and +# not (d.protected and +# d.removable)] +# + disks = [] + for d in devices: + if d.isDisk and not d.format.hidden and not (d.protected and d.removable): + # unformatted DASDs are detected with a size of 0, but they should + # still show up as valid disks if this function is called, since we + # can still use them; anaconda will know how to handle them, so they + # don't need to be ignored anymore + if d.type == "dasd": + disks.append(d) + elif d.size > 0: + disks.append(d) + else: disks = [] disks.append(FakeDisk("sda", size=300000, free=10000, serial="00001",
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
Since the installer can handle unformatted DASDs, all of them should be viewable and selectable from the storage spoke.
Resolves: rhbz#1064423
pyanaconda/ui/lib/disks.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/pyanaconda/ui/lib/disks.py b/pyanaconda/ui/lib/disks.py index 486208b..010b9af 100644 --- a/pyanaconda/ui/lib/disks.py +++ b/pyanaconda/ui/lib/disks.py @@ -53,11 +53,24 @@ def getDisks(devicetree, fake=False): if not flags.imageInstall: devices += devicetree._hidden
disks = [d for d in devices if d.isDisk andd.mediaPresent andnot d.format.hidden andnot (d.protected andd.removable)]+# disks = [d for d in devices if d.isDisk and +# d.mediaPresent and +# not d.format.hidden and +# not (d.protected and +# d.removable)] +#
We probably don't need this nice piece of history anymore. :)
disks = []for d in devices:if d.isDisk and not d.format.hidden and not (d.protected and d.removable):
What about the 'd.mediaPresent' check?
On Tue, Feb 25, 2014 at 01:04:07PM +0100, Vratislav Podzimek wrote:
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
Since the installer can handle unformatted DASDs, all of them should be viewable and selectable from the storage spoke.
Resolves: rhbz#1064423
pyanaconda/ui/lib/disks.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/pyanaconda/ui/lib/disks.py b/pyanaconda/ui/lib/disks.py index 486208b..010b9af 100644 --- a/pyanaconda/ui/lib/disks.py +++ b/pyanaconda/ui/lib/disks.py @@ -53,11 +53,24 @@ def getDisks(devicetree, fake=False): if not flags.imageInstall: devices += devicetree._hidden
disks = [d for d in devices if d.isDisk andd.mediaPresent andnot d.format.hidden andnot (d.protected andd.removable)]+# disks = [d for d in devices if d.isDisk and +# d.mediaPresent and +# not d.format.hidden and +# not (d.protected and +# d.removable)] +#
We probably don't need this nice piece of history anymore. :)
Ha, very true. ;) Removed.
disks = []for d in devices:if d.isDisk and not d.format.hidden and not (d.protected and d.removable):What about the 'd.mediaPresent' check?
Ahh, right. So unformatted DASDs will fail that and the size > 0 check, hence why I just ignore those if disk.type == "dasd". But I could make my conditional instead:
if d.type == "dasd": disks.append(d) elif d.size > 0 and d.mediaPresent: disks.append(d)
Samantha
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
On Tue, 2014-02-25 at 08:59 -0500, Samantha N. Bueno wrote:
On Tue, Feb 25, 2014 at 01:04:07PM +0100, Vratislav Podzimek wrote:
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
Since the installer can handle unformatted DASDs, all of them should be viewable and selectable from the storage spoke.
Resolves: rhbz#1064423
pyanaconda/ui/lib/disks.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/pyanaconda/ui/lib/disks.py b/pyanaconda/ui/lib/disks.py index 486208b..010b9af 100644 --- a/pyanaconda/ui/lib/disks.py +++ b/pyanaconda/ui/lib/disks.py @@ -53,11 +53,24 @@ def getDisks(devicetree, fake=False): if not flags.imageInstall: devices += devicetree._hidden
disks = [d for d in devices if d.isDisk andd.mediaPresent andnot d.format.hidden andnot (d.protected andd.removable)]+# disks = [d for d in devices if d.isDisk and +# d.mediaPresent and +# not d.format.hidden and +# not (d.protected and +# d.removable)] +#
We probably don't need this nice piece of history anymore. :)
Ha, very true. ;) Removed.
disks = []for d in devices:if d.isDisk and not d.format.hidden and not (d.protected and d.removable):What about the 'd.mediaPresent' check?
Ahh, right. So unformatted DASDs will fail that and the size > 0 check, hence why I just ignore those if disk.type == "dasd". But I could make my conditional instead:
if d.type == "dasd": disks.append(d) elif d.size > 0 and d.mediaPresent: disks.append(d)
This looks good to me.
In both graphical and text, if a user selects unformatted DASDs from the local disk store, they are formatted when the user opts to proceed (either by clicking 'Done' in the GUI or 'c' to continue in the TUI).
Following dasdfmt, storage initialization is run again in order to properly add the new DASDs to the devicetree and seen by the installer.
Resolves:rhbz#1064423 --- pyanaconda/constants.py | 1 + pyanaconda/ui/gui/spokes/storage.py | 73 ++++++++++++++++++++++++++++++++++++- pyanaconda/ui/tui/spokes/storage.py | 44 ++++++++++++++++++++-- 3 files changed, 113 insertions(+), 5 deletions(-)
diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py index cf8a8ea..8de29a9 100644 --- a/pyanaconda/constants.py +++ b/pyanaconda/constants.py @@ -131,6 +131,7 @@ THREAD_ISCSI_LOGIN = "AnaIscsiLoginThread" THREAD_GEOLOCATION_REFRESH = "AnaGeolocationRefreshThread" THREAD_DATE_TIME = "AnaDateTimeThread" THREAD_TIME_INIT = "AnaTimeInitThread" +THREAD_DASDFMT = "AnaDasdfmtThread"
# Geolocation constants
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index bdd48ef..a42c540 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -50,17 +50,20 @@ from pyanaconda.ui.gui.spokes.lib.cart import SelectedDisksDialog from pyanaconda.ui.gui.spokes.lib.passphrase import PassphraseDialog from pyanaconda.ui.gui.spokes.lib.detailederror import DetailedErrorDialog from pyanaconda.ui.gui.spokes.lib.resize import ResizeDialog +from pyanaconda.ui.gui.spokes.lib.dasdfmt import DasdFormatDialog from pyanaconda.ui.gui.categories.system import SystemCategory -from pyanaconda.ui.gui.utils import enlightbox, gtk_call_once, gtk_action_wait +from pyanaconda.ui.gui.utils import enlightbox, gtk_call_once, gtk_action_wait, ignoreEscape
from pyanaconda.kickstart import doKickstartStorage, getAvailableDiskSpace +from blivet import storageInitialize, arch from blivet.size import Size from blivet.devices import MultipathDevice -from blivet.errors import StorageError +from blivet.errors import StorageError, DasdFormatError from blivet.errors import SanityError from blivet.errors import SanityWarning from blivet.platform import platform from blivet.devicelibs import swap as swap_lib +from blivet.devicelibs.dasd import make_unformatted_dasd_list, format_dasd from pyanaconda.threads import threadMgr, AnacondaThread from pyanaconda.product import productName from pyanaconda.flags import flags @@ -292,6 +295,11 @@ class StorageSpoke(NormalSpoke, StorageChecker): self.autoPartType = None self.clearPartType = CLEARPART_TYPE_NONE
+ if self.data.zerombr.zerombr and arch.isS390(): + # run dasdfmt on any unformatted DASDs automatically + threadMgr.add(AnacondaThread(name=constants.THREAD_DASDFMT, + target=self.run_dasdfmt)) + self._previous_autopart = False
self._last_clicked_overview = None @@ -357,6 +365,9 @@ class StorageSpoke(NormalSpoke, StorageChecker): def _doExecute(self): self._ready = False hubQ.send_not_ready(self.__class__.__name__) + # on the off-chance dasdfmt is running, bail + if threadMgr.get(constants.THREAD_DASDFMT): + return hubQ.send_message(self.__class__.__name__, _("Saving storage configuration...")) try: doKickstartStorage(self.storage, self.data, self.instclass) @@ -409,6 +420,8 @@ class StorageSpoke(NormalSpoke, StorageChecker):
if not self._confirmed: msg = _("Not configured") + elif threadMgr.get(constants.THREAD_DASDFMT): + msg = _("Formatting DASDs") elif flags.automatedInstall and not self.storage.rootDevice: return msg elif self.data.ignoredisk.onlyuse: @@ -681,6 +694,36 @@ class StorageSpoke(NormalSpoke, StorageChecker): if not selected and name in self.selected_disks: self.selected_disks.remove(name)
+ def run_dasdfmt(self): + """ + Though the same function exists in pyanaconda.ui.gui.spokes.lib.dasdfmt, + this instance doesn't include any of the UI pieces and should only + really be getting called on ks installations with "zerombr". + """ + to_format = make_unformatted_dasd_list(self.selected_disks) + if len(to_format) == 0: + # nothing to do here; bail + return + + for disk in to_format: + try: + format_dasd(disk) + except DasdFormatError as err: + # Log errors if formatting fails, but don't halt the installer + log.error(str(err)) + continue + + # when finished formatting we need to reinitialize storage + # however, check that we don't already have a storage thread running + if threadMgr.get(constants.THREAD_STORAGE): + threadMgr.wait(constants.THREAD_STORAGE) + + protectedNames = map(lambda d: d.name, self.storage.protectedDevices) + threadMgr.add(AnacondaThread(name=constants.THREAD_STORAGE, + target=storageInitialize, + args=(self.storage, self.data, protectedNames))) + threadMgr.wait(constants.THREAD_STORAGE) + # signal handlers def on_summary_clicked(self, button): # show the selected disks dialog @@ -769,6 +812,32 @@ class StorageSpoke(NormalSpoke, StorageChecker): NormalSpoke.on_back_clicked(self, button) return
+ if arch.isS390(): + dasds = make_unformatted_dasd_list(self.selected_disks) + if len(dasds) > 0: + dialog = DasdFormatDialog(self.data, self.storage, dasds) + ignoreEscape(dialog.window) + rc = self.run_lightbox_dialog(dialog) + if rc == 1: + # User hit OK on the dialog, indicating they stayed on the + # dialog until formatting completed and now needs to go back + # to the main storage spoke. + dialog.window.destroy() + # make sure we stay on the storage spoke and don't return to + # the summary hub + self.skipTo = "StorageSpoke" + self.refresh() + elif rc == 2: + # User clicked uri to return to hub. + NormalSpoke.on_back_clicked(self, button) + return + elif rc != 2: + # User either hit cancel on the dialog or closed it via escape, so + # there was no formatting done. + # NOTE: rc == 2 means the user clicked on the link that takes them + # back to the hub. + return + # Figure out if the existing disk labels will work on this platform # you need to have at least one of the platform's labels in order for # any of the free space to be useful. diff --git a/pyanaconda/ui/tui/spokes/storage.py b/pyanaconda/ui/tui/spokes/storage.py index b4d40f1..c20b649 100644 --- a/pyanaconda/ui/tui/spokes/storage.py +++ b/pyanaconda/ui/tui/spokes/storage.py @@ -27,15 +27,17 @@ from pyanaconda.ui.tui.spokes import NormalTUISpoke from pyanaconda.ui.tui.simpleline import TextWidget, CheckboxWidget
from pykickstart.constants import AUTOPART_TYPE_LVM, AUTOPART_TYPE_BTRFS, AUTOPART_TYPE_PLAIN +from blivet import storageInitialize, arch from blivet.size import Size -from blivet.errors import StorageError +from blivet.errors import StorageError, DasdFormatError from blivet.errors import SanityError from blivet.errors import SanityWarning from blivet.devices import DASDDevice, FcoeDiskDevice, iScsiDiskDevice, MultipathDevice, ZFCPDiskDevice +from blivet.devicelibs.dasd import format_dasd, make_unformatted_dasd_list from pyanaconda.flags import flags from pyanaconda.kickstart import doKickstartStorage from pyanaconda.threads import threadMgr, AnacondaThread -from pyanaconda.constants import THREAD_STORAGE, THREAD_STORAGE_WATCHER +from pyanaconda.constants import THREAD_STORAGE, THREAD_STORAGE_WATCHER, THREAD_DASDFMT from pyanaconda.i18n import _, P_, N_ from pyanaconda.bootloader import BootLoaderError
@@ -75,6 +77,9 @@ class StorageSpoke(NormalTUISpoke): self.errors = [] self.warnings = []
+ if self.data.zerombr.zerombr and arch.isS390(): + self.run_dasdfmt() + if not flags.automatedInstall: # default to using autopart for interactive installs self.data.autopart.autopart = True @@ -89,7 +94,7 @@ class StorageSpoke(NormalTUISpoke): def ready(self): # By default, the storage spoke is not ready. We have to wait until # storageInitialize is done. - return self._ready and not threadMgr.get(THREAD_STORAGE_WATCHER) + return self._ready and not (threadMgr.get(THREAD_STORAGE_WATCHER) or threadMgr.get(THREAD_DASDFMT))
@property def mandatory(self): @@ -231,6 +236,13 @@ class StorageSpoke(NormalTUISpoke):
if key == "c": if self.selected_disks: + # check selected disks to see if we have any unformatted DASDs + # if we're on s390x, since they need to be formatted before we + # can use them. + if arch.isS390(): + self.run_dasdfmt() + return None + newspoke = AutoPartSpoke(self.app, self.data, self.storage, self.payload, self.instclass) self.app.switch_screen_modal(newspoke) @@ -247,6 +259,32 @@ class StorageSpoke(NormalTUISpoke): except (ValueError, KeyError, IndexError): return key
+ def run_dasdfmt(self): + """ + This generates the list of DASDs requiring dasdfmt and runs dasdfmt + against them. + """ + to_format = make_unformatted_dasd_list(self.selected_disks) + if len(to_format) == 0: + # nothing to do here; bail + return + + for disk in to_format: + try: + print(_("Formatting /dev/%s. This may take a moment." % disk)) + format_dasd(disk) + except DasdFormatError as err: + # Log errors if formatting fails, but don't halt the installer + log.error(str(err)) + continue + + # when finished formatting we need to reinitialize storage + protectedNames = map(lambda d: d.name, self.storage.protectedDevices) + threadMgr.add(AnacondaThread(name=THREAD_STORAGE, + target=storageInitialize, + args=(self.storage, self.data, protectedNames))) + threadMgr.wait(THREAD_STORAGE) + def apply(self): self.autopart = self.data.autopart.autopart self.data.ignoredisk.onlyuse = self.selected_disks[:]
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
In both graphical and text, if a user selects unformatted DASDs from the local disk store, they are formatted when the user opts to proceed (either by clicking 'Done' in the GUI or 'c' to continue in the TUI).
Following dasdfmt, storage initialization is run again in order to properly add the new DASDs to the devicetree and seen by the installer.
Resolves:rhbz#1064423
pyanaconda/constants.py | 1 + pyanaconda/ui/gui/spokes/storage.py | 73 ++++++++++++++++++++++++++++++++++++- pyanaconda/ui/tui/spokes/storage.py | 44 ++++++++++++++++++++-- 3 files changed, 113 insertions(+), 5 deletions(-)
diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py index cf8a8ea..8de29a9 100644 --- a/pyanaconda/constants.py +++ b/pyanaconda/constants.py @@ -131,6 +131,7 @@ THREAD_ISCSI_LOGIN = "AnaIscsiLoginThread" THREAD_GEOLOCATION_REFRESH = "AnaGeolocationRefreshThread" THREAD_DATE_TIME = "AnaDateTimeThread" THREAD_TIME_INIT = "AnaTimeInitThread" +THREAD_DASDFMT = "AnaDasdfmtThread"
# Geolocation constants
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index bdd48ef..a42c540 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -50,17 +50,20 @@ from pyanaconda.ui.gui.spokes.lib.cart import SelectedDisksDialog from pyanaconda.ui.gui.spokes.lib.passphrase import PassphraseDialog from pyanaconda.ui.gui.spokes.lib.detailederror import DetailedErrorDialog from pyanaconda.ui.gui.spokes.lib.resize import ResizeDialog +from pyanaconda.ui.gui.spokes.lib.dasdfmt import DasdFormatDialog from pyanaconda.ui.gui.categories.system import SystemCategory -from pyanaconda.ui.gui.utils import enlightbox, gtk_call_once, gtk_action_wait +from pyanaconda.ui.gui.utils import enlightbox, gtk_call_once, gtk_action_wait, ignoreEscape
from pyanaconda.kickstart import doKickstartStorage, getAvailableDiskSpace +from blivet import storageInitialize, arch from blivet.size import Size from blivet.devices import MultipathDevice -from blivet.errors import StorageError +from blivet.errors import StorageError, DasdFormatError from blivet.errors import SanityError from blivet.errors import SanityWarning from blivet.platform import platform from blivet.devicelibs import swap as swap_lib +from blivet.devicelibs.dasd import make_unformatted_dasd_list, format_dasd from pyanaconda.threads import threadMgr, AnacondaThread from pyanaconda.product import productName from pyanaconda.flags import flags @@ -292,6 +295,11 @@ class StorageSpoke(NormalSpoke, StorageChecker): self.autoPartType = None self.clearPartType = CLEARPART_TYPE_NONE
if self.data.zerombr.zerombr and arch.isS390():# run dasdfmt on any unformatted DASDs automaticallythreadMgr.add(AnacondaThread(name=constants.THREAD_DASDFMT,target=self.run_dasdfmt))self._previous_autopart = False self._last_clicked_overview = None@@ -357,6 +365,9 @@ class StorageSpoke(NormalSpoke, StorageChecker): def _doExecute(self): self._ready = False hubQ.send_not_ready(self.__class__.__name__)
# on the off-chance dasdfmt is running, bailif threadMgr.get(constants.THREAD_DASDFMT):return hubQ.send_message(self.__class__.__name__, _("Saving storage configuration...")) try: doKickstartStorage(self.storage, self.data, self.instclass)@@ -409,6 +420,8 @@ class StorageSpoke(NormalSpoke, StorageChecker):
if not self._confirmed: msg = _("Not configured")
elif threadMgr.get(constants.THREAD_DASDFMT):msg = _("Formatting DASDs") elif flags.automatedInstall and not self.storage.rootDevice: return msg elif self.data.ignoredisk.onlyuse:@@ -681,6 +694,36 @@ class StorageSpoke(NormalSpoke, StorageChecker): if not selected and name in self.selected_disks: self.selected_disks.remove(name)
- def run_dasdfmt(self):
"""Though the same function exists in pyanaconda.ui.gui.spokes.lib.dasdfmt,this instance doesn't include any of the UI pieces and should onlyreally be getting called on ks installations with "zerombr"."""to_format = make_unformatted_dasd_list(self.selected_disks)if len(to_format) == 0:# nothing to do here; bailreturnfor disk in to_format:try:format_dasd(disk)except DasdFormatError as err:# Log errors if formatting fails, but don't halt the installerlog.error(str(err))continue# when finished formatting we need to reinitialize storage# however, check that we don't already have a storage thread runningif threadMgr.get(constants.THREAD_STORAGE):threadMgr.wait(constants.THREAD_STORAGE)
I believe you can call wait directly, it should be no-op if there is no such thread. But shouldn't this wait happen in the first place before formatting DASDs?
protectedNames = map(lambda d: d.name, self.storage.protectedDevices)
You can use a list comprehension here: [d.name for d in self.storage.protectedDevices]
threadMgr.add(AnacondaThread(name=constants.THREAD_STORAGE,target=storageInitialize,args=(self.storage, self.data, protectedNames)))threadMgr.wait(constants.THREAD_STORAGE)- # signal handlers def on_summary_clicked(self, button): # show the selected disks dialog
@@ -769,6 +812,32 @@ class StorageSpoke(NormalSpoke, StorageChecker): NormalSpoke.on_back_clicked(self, button) return
if arch.isS390():dasds = make_unformatted_dasd_list(self.selected_disks)if len(dasds) > 0:dialog = DasdFormatDialog(self.data, self.storage, dasds)ignoreEscape(dialog.window)rc = self.run_lightbox_dialog(dialog)if rc == 1:# User hit OK on the dialog, indicating they stayed on the# dialog until formatting completed and now needs to go back# to the main storage spoke.dialog.window.destroy()# make sure we stay on the storage spoke and don't return to# the summary hubself.skipTo = "StorageSpoke"self.refresh()
I think refresh will get called when the spoke is entered again due to skipTo.
elif rc == 2:# User clicked uri to return to hub.NormalSpoke.on_back_clicked(self, button)returnelif rc != 2:# User either hit cancel on the dialog or closed it via escape, so# there was no formatting done.# NOTE: rc == 2 means the user clicked on the link that takes them# back to the hub.return# Figure out if the existing disk labels will work on this platform # you need to have at least one of the platform's labels in order for # any of the free space to be useful.diff --git a/pyanaconda/ui/tui/spokes/storage.py b/pyanaconda/ui/tui/spokes/storage.py index b4d40f1..c20b649 100644 --- a/pyanaconda/ui/tui/spokes/storage.py +++ b/pyanaconda/ui/tui/spokes/storage.py @@ -27,15 +27,17 @@ from pyanaconda.ui.tui.spokes import NormalTUISpoke from pyanaconda.ui.tui.simpleline import TextWidget, CheckboxWidget
from pykickstart.constants import AUTOPART_TYPE_LVM, AUTOPART_TYPE_BTRFS, AUTOPART_TYPE_PLAIN +from blivet import storageInitialize, arch from blivet.size import Size -from blivet.errors import StorageError +from blivet.errors import StorageError, DasdFormatError from blivet.errors import SanityError from blivet.errors import SanityWarning from blivet.devices import DASDDevice, FcoeDiskDevice, iScsiDiskDevice, MultipathDevice, ZFCPDiskDevice +from blivet.devicelibs.dasd import format_dasd, make_unformatted_dasd_list from pyanaconda.flags import flags from pyanaconda.kickstart import doKickstartStorage from pyanaconda.threads import threadMgr, AnacondaThread -from pyanaconda.constants import THREAD_STORAGE, THREAD_STORAGE_WATCHER +from pyanaconda.constants import THREAD_STORAGE, THREAD_STORAGE_WATCHER, THREAD_DASDFMT from pyanaconda.i18n import _, P_, N_ from pyanaconda.bootloader import BootLoaderError
@@ -75,6 +77,9 @@ class StorageSpoke(NormalTUISpoke): self.errors = [] self.warnings = []
if self.data.zerombr.zerombr and arch.isS390():self.run_dasdfmt()if not flags.automatedInstall: # default to using autopart for interactive installs self.data.autopart.autopart = True@@ -89,7 +94,7 @@ class StorageSpoke(NormalTUISpoke): def ready(self): # By default, the storage spoke is not ready. We have to wait until # storageInitialize is done.
return self._ready and not threadMgr.get(THREAD_STORAGE_WATCHER)
return self._ready and not (threadMgr.get(THREAD_STORAGE_WATCHER) or threadMgr.get(THREAD_DASDFMT))@property def mandatory(self):
@@ -231,6 +236,13 @@ class StorageSpoke(NormalTUISpoke):
if key == "c": if self.selected_disks:
# check selected disks to see if we have any unformatted DASDs# if we're on s390x, since they need to be formatted before we# can use them.if arch.isS390():self.run_dasdfmt()return Nonenewspoke = AutoPartSpoke(self.app, self.data, self.storage, self.payload, self.instclass) self.app.switch_screen_modal(newspoke)@@ -247,6 +259,32 @@ class StorageSpoke(NormalTUISpoke): except (ValueError, KeyError, IndexError): return key
- def run_dasdfmt(self):
"""This generates the list of DASDs requiring dasdfmt and runs dasdfmtagainst them."""to_format = make_unformatted_dasd_list(self.selected_disks)if len(to_format) == 0:
'if not to_format' would do the same.
# nothing to do here; bailreturnfor disk in to_format:try:print(_("Formatting /dev/%s. This may take a moment." % disk))format_dasd(disk)except DasdFormatError as err:# Log errors if formatting fails, but don't halt the installerlog.error(str(err))continue# when finished formatting we need to reinitialize storageprotectedNames = map(lambda d: d.name, self.storage.protectedDevices)
The same list comprehension as in the GUI could be used here.
threadMgr.add(AnacondaThread(name=THREAD_STORAGE,target=storageInitialize,args=(self.storage, self.data, protectedNames)))threadMgr.wait(THREAD_STORAGE)- def apply(self): self.autopart = self.data.autopart.autopart self.data.ignoredisk.onlyuse = self.selected_disks[:]
On Tue, Feb 25, 2014 at 01:14:58PM +0100, Vratislav Podzimek wrote:
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
In both graphical and text, if a user selects unformatted DASDs from the local disk store, they are formatted when the user opts to proceed (either by clicking 'Done' in the GUI or 'c' to continue in the TUI).
Following dasdfmt, storage initialization is run again in order to properly add the new DASDs to the devicetree and seen by the installer.
Resolves:rhbz#1064423
pyanaconda/constants.py | 1 + pyanaconda/ui/gui/spokes/storage.py | 73 ++++++++++++++++++++++++++++++++++++- pyanaconda/ui/tui/spokes/storage.py | 44 ++++++++++++++++++++-- 3 files changed, 113 insertions(+), 5 deletions(-)
diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py index cf8a8ea..8de29a9 100644 --- a/pyanaconda/constants.py +++ b/pyanaconda/constants.py @@ -131,6 +131,7 @@ THREAD_ISCSI_LOGIN = "AnaIscsiLoginThread" THREAD_GEOLOCATION_REFRESH = "AnaGeolocationRefreshThread" THREAD_DATE_TIME = "AnaDateTimeThread" THREAD_TIME_INIT = "AnaTimeInitThread" +THREAD_DASDFMT = "AnaDasdfmtThread"
# Geolocation constants
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py index bdd48ef..a42c540 100644 --- a/pyanaconda/ui/gui/spokes/storage.py +++ b/pyanaconda/ui/gui/spokes/storage.py @@ -50,17 +50,20 @@ from pyanaconda.ui.gui.spokes.lib.cart import SelectedDisksDialog from pyanaconda.ui.gui.spokes.lib.passphrase import PassphraseDialog from pyanaconda.ui.gui.spokes.lib.detailederror import DetailedErrorDialog from pyanaconda.ui.gui.spokes.lib.resize import ResizeDialog +from pyanaconda.ui.gui.spokes.lib.dasdfmt import DasdFormatDialog from pyanaconda.ui.gui.categories.system import SystemCategory -from pyanaconda.ui.gui.utils import enlightbox, gtk_call_once, gtk_action_wait +from pyanaconda.ui.gui.utils import enlightbox, gtk_call_once, gtk_action_wait, ignoreEscape
from pyanaconda.kickstart import doKickstartStorage, getAvailableDiskSpace +from blivet import storageInitialize, arch from blivet.size import Size from blivet.devices import MultipathDevice -from blivet.errors import StorageError +from blivet.errors import StorageError, DasdFormatError from blivet.errors import SanityError from blivet.errors import SanityWarning from blivet.platform import platform from blivet.devicelibs import swap as swap_lib +from blivet.devicelibs.dasd import make_unformatted_dasd_list, format_dasd from pyanaconda.threads import threadMgr, AnacondaThread from pyanaconda.product import productName from pyanaconda.flags import flags @@ -292,6 +295,11 @@ class StorageSpoke(NormalSpoke, StorageChecker): self.autoPartType = None self.clearPartType = CLEARPART_TYPE_NONE
if self.data.zerombr.zerombr and arch.isS390():# run dasdfmt on any unformatted DASDs automaticallythreadMgr.add(AnacondaThread(name=constants.THREAD_DASDFMT,target=self.run_dasdfmt))self._previous_autopart = False self._last_clicked_overview = None@@ -357,6 +365,9 @@ class StorageSpoke(NormalSpoke, StorageChecker): def _doExecute(self): self._ready = False hubQ.send_not_ready(self.__class__.__name__)
# on the off-chance dasdfmt is running, bailif threadMgr.get(constants.THREAD_DASDFMT):return hubQ.send_message(self.__class__.__name__, _("Saving storage configuration...")) try: doKickstartStorage(self.storage, self.data, self.instclass)@@ -409,6 +420,8 @@ class StorageSpoke(NormalSpoke, StorageChecker):
if not self._confirmed: msg = _("Not configured")
elif threadMgr.get(constants.THREAD_DASDFMT):msg = _("Formatting DASDs") elif flags.automatedInstall and not self.storage.rootDevice: return msg elif self.data.ignoredisk.onlyuse:@@ -681,6 +694,36 @@ class StorageSpoke(NormalSpoke, StorageChecker): if not selected and name in self.selected_disks: self.selected_disks.remove(name)
- def run_dasdfmt(self):
"""Though the same function exists in pyanaconda.ui.gui.spokes.lib.dasdfmt,this instance doesn't include any of the UI pieces and should onlyreally be getting called on ks installations with "zerombr"."""to_format = make_unformatted_dasd_list(self.selected_disks)if len(to_format) == 0:# nothing to do here; bailreturnfor disk in to_format:try:format_dasd(disk)except DasdFormatError as err:# Log errors if formatting fails, but don't halt the installerlog.error(str(err))continue# when finished formatting we need to reinitialize storage# however, check that we don't already have a storage thread runningif threadMgr.get(constants.THREAD_STORAGE):threadMgr.wait(constants.THREAD_STORAGE)I believe you can call wait directly, it should be no-op if there is no such thread. But shouldn't this wait happen in the first place before formatting DASDs?
Ah yes, you are right, I will delete the get() since it's redundant.
protectedNames = map(lambda d: d.name, self.storage.protectedDevices)You can use a list comprehension here: [d.name for d in self.storage.protectedDevices]
I think you may have mentioned that last time and thought I'd changed that; will do so now. Pylint was throwing a warning about that anyway. :)
threadMgr.add(AnacondaThread(name=constants.THREAD_STORAGE,target=storageInitialize,args=(self.storage, self.data, protectedNames)))threadMgr.wait(constants.THREAD_STORAGE)- # signal handlers def on_summary_clicked(self, button): # show the selected disks dialog
@@ -769,6 +812,32 @@ class StorageSpoke(NormalSpoke, StorageChecker): NormalSpoke.on_back_clicked(self, button) return
if arch.isS390():dasds = make_unformatted_dasd_list(self.selected_disks)if len(dasds) > 0:dialog = DasdFormatDialog(self.data, self.storage, dasds)ignoreEscape(dialog.window)rc = self.run_lightbox_dialog(dialog)if rc == 1:# User hit OK on the dialog, indicating they stayed on the# dialog until formatting completed and now needs to go back# to the main storage spoke.dialog.window.destroy()# make sure we stay on the storage spoke and don't return to# the summary hubself.skipTo = "StorageSpoke"self.refresh()I think refresh will get called when the spoke is entered again due to skipTo.
I thought it would as well and tried that initially, but it does not get called. Thought about adding a comment; I'll add one now.
elif rc == 2:# User clicked uri to return to hub.NormalSpoke.on_back_clicked(self, button)returnelif rc != 2:# User either hit cancel on the dialog or closed it via escape, so# there was no formatting done.# NOTE: rc == 2 means the user clicked on the link that takes them# back to the hub.return# Figure out if the existing disk labels will work on this platform # you need to have at least one of the platform's labels in order for # any of the free space to be useful.diff --git a/pyanaconda/ui/tui/spokes/storage.py b/pyanaconda/ui/tui/spokes/storage.py index b4d40f1..c20b649 100644 --- a/pyanaconda/ui/tui/spokes/storage.py +++ b/pyanaconda/ui/tui/spokes/storage.py @@ -27,15 +27,17 @@ from pyanaconda.ui.tui.spokes import NormalTUISpoke from pyanaconda.ui.tui.simpleline import TextWidget, CheckboxWidget
from pykickstart.constants import AUTOPART_TYPE_LVM, AUTOPART_TYPE_BTRFS, AUTOPART_TYPE_PLAIN +from blivet import storageInitialize, arch from blivet.size import Size -from blivet.errors import StorageError +from blivet.errors import StorageError, DasdFormatError from blivet.errors import SanityError from blivet.errors import SanityWarning from blivet.devices import DASDDevice, FcoeDiskDevice, iScsiDiskDevice, MultipathDevice, ZFCPDiskDevice +from blivet.devicelibs.dasd import format_dasd, make_unformatted_dasd_list from pyanaconda.flags import flags from pyanaconda.kickstart import doKickstartStorage from pyanaconda.threads import threadMgr, AnacondaThread -from pyanaconda.constants import THREAD_STORAGE, THREAD_STORAGE_WATCHER +from pyanaconda.constants import THREAD_STORAGE, THREAD_STORAGE_WATCHER, THREAD_DASDFMT from pyanaconda.i18n import _, P_, N_ from pyanaconda.bootloader import BootLoaderError
@@ -75,6 +77,9 @@ class StorageSpoke(NormalTUISpoke): self.errors = [] self.warnings = []
if self.data.zerombr.zerombr and arch.isS390():self.run_dasdfmt()if not flags.automatedInstall: # default to using autopart for interactive installs self.data.autopart.autopart = True@@ -89,7 +94,7 @@ class StorageSpoke(NormalTUISpoke): def ready(self): # By default, the storage spoke is not ready. We have to wait until # storageInitialize is done.
return self._ready and not threadMgr.get(THREAD_STORAGE_WATCHER)
return self._ready and not (threadMgr.get(THREAD_STORAGE_WATCHER) or threadMgr.get(THREAD_DASDFMT))@property def mandatory(self):
@@ -231,6 +236,13 @@ class StorageSpoke(NormalTUISpoke):
if key == "c": if self.selected_disks:
# check selected disks to see if we have any unformatted DASDs# if we're on s390x, since they need to be formatted before we# can use them.if arch.isS390():self.run_dasdfmt()return Nonenewspoke = AutoPartSpoke(self.app, self.data, self.storage, self.payload, self.instclass) self.app.switch_screen_modal(newspoke)@@ -247,6 +259,32 @@ class StorageSpoke(NormalTUISpoke): except (ValueError, KeyError, IndexError): return key
- def run_dasdfmt(self):
"""This generates the list of DASDs requiring dasdfmt and runs dasdfmtagainst them."""to_format = make_unformatted_dasd_list(self.selected_disks)if len(to_format) == 0:'if not to_format' would do the same.
Yes, fixed locally and in the few other places I have that same line.
# nothing to do here; bailreturnfor disk in to_format:try:print(_("Formatting /dev/%s. This may take a moment." % disk))format_dasd(disk)except DasdFormatError as err:# Log errors if formatting fails, but don't halt the installerlog.error(str(err))continue# when finished formatting we need to reinitialize storageprotectedNames = map(lambda d: d.name, self.storage.protectedDevices)The same list comprehension as in the GUI could be used here.
Fixed locally.
threadMgr.add(AnacondaThread(name=THREAD_STORAGE,target=storageInitialize,args=(self.storage, self.data, protectedNames)))threadMgr.wait(THREAD_STORAGE)- def apply(self): self.autopart = self.data.autopart.autopart self.data.ignoredisk.onlyuse = self.selected_disks[:]
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Samantha, does it mean that anaconda will format all unformatted disks without asking? It looks so from the video and in my opinion this should not happen, anaconda should ask.
Another issue - after the formatting is finished, you are returned to main hub, so you have to enter the Installation destination spoke again - is it necessary to return to main hub? Also note that main hub says "No disks selected", but the "Begin Installation" button is enabled, which also doesn't look right.
Thanks for working on the "zerombr" kickstart command.
Jan
On 02/25/2014 10:30 AM, Samantha N. Bueno wrote:
This includes fixes suggested from the first review. The flow of this is also a bit different from when I first posted as well, thanks to discussions with dlehman and mizmo (read: simpler and less clunky). The text install is basically the same, but for an updated screencast of the graphical version here's a link: http://sbueno.fedorapeople.org/1001070-graphical-25feb.webm
jstodola mentioned the "zerombr" kickstart command last time; support for that is here; if "zerombr" is in a ks file and unformatted DASDs are specified, they automatically have dasdfmt run against them.
A few points that are sort of minor but which bug me, all graphical:
The link on the dialog box back to the summary hub doesn't work, so I sort of force it myself in what's probably a less correct way. I'm not sure what format the URI should be, so if anyone knows, let me know.
Again following a return to the summary hub, while letting dasdfmt run in the background, the UI sort of freezes up; I'm guessing I need some sort of decorator somewhere, maybe?
I think my thread handling is jumbled and awkward in places (GUI storage), then again trying to cleanly work in the "zerombr" command was no picnic. I'd be greatly in favor of only allowing "zerombr" as valid if in text mode; the graphical code is hideous IMO.
On Tue, 2014-02-25 at 13:06 +0100, Jan Stodola wrote:
Samantha, does it mean that anaconda will format all unformatted disks without asking? It looks so from the video and in my opinion this should not happen, anaconda should ask.
I believe the video shows the 'zerombr' case, but I'm not sure.
On Tue, Feb 25, 2014 at 01:06:53PM +0100, Jan Stodola wrote:
Samantha, does it mean that anaconda will format all unformatted disks without asking? It looks so from the video and in my opinion this should not happen, anaconda should ask.
All disks, no; just the ones which are selected. Since one could have many disks which are unformatted, and since it is time-consuming to run dasdfmt especially as your number of disks grows, it was decided to not blanket dasdfmt all of them, especially since a user might not wish to use all of them.
Another issue - after the formatting is finished, you are returned to main hub, so you have to enter the Installation destination spoke again - is it necessary to return to main hub? Also note that main hub says "No disks selected", but the "Begin Installation" button is enabled, which also doesn't look right.
Oops, I forgot this screencast had that trailing error. Sorry -- the user is definitely not returned to the main hub after the disks are formatted. The storage spoke is instead refreshed, so yes, the user stays there.
I did not see the button sensitivity for 'Begin Installation' had changed; thanks for pointing that out, I will look into why that happens.
Thanks for working on the "zerombr" kickstart command.
No problem, thanks for reminding me about it. :)
Samantha
Jan
On 02/25/2014 10:30 AM, Samantha N. Bueno wrote:
This includes fixes suggested from the first review. The flow of this is also a bit different from when I first posted as well, thanks to discussions with dlehman and mizmo (read: simpler and less clunky). The text install is basically the same, but for an updated screencast of the graphical version here's a link: http://sbueno.fedorapeople.org/1001070-graphical-25feb.webm
jstodola mentioned the "zerombr" kickstart command last time; support for that is here; if "zerombr" is in a ks file and unformatted DASDs are specified, they automatically have dasdfmt run against them.
A few points that are sort of minor but which bug me, all graphical:
The link on the dialog box back to the summary hub doesn't work, so I sort of force it myself in what's probably a less correct way. I'm not sure what format the URI should be, so if anyone knows, let me know.
Again following a return to the summary hub, while letting dasdfmt run in the background, the UI sort of freezes up; I'm guessing I need some sort of decorator somewhere, maybe?
I think my thread handling is jumbled and awkward in places (GUI storage), then again trying to cleanly work in the "zerombr" command was no picnic. I'd be greatly in favor of only allowing "zerombr" as valid if in text mode; the graphical code is hideous IMO.
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
On 02/25/2014 03:57 PM, Samantha N. Bueno wrote:
On Tue, Feb 25, 2014 at 01:06:53PM +0100, Jan Stodola wrote:
Samantha, does it mean that anaconda will format all unformatted disks without asking? It looks so from the video and in my opinion this should not happen, anaconda should ask.
All disks, no; just the ones which are selected. Since one could have many disks which are unformatted, and since it is time-consuming to run dasdfmt especially as your number of disks grows, it was decided to not blanket dasdfmt all of them, especially since a user might not wish to use all of them.
I meant a bit different issue - I will describe better:
The main hub says "We won't touch your disks until you click 'Begin Installation'". In the Installation Destination spoke you selected 4 disks and clicked "Done". What happened: anaconda started formatting selected unformatted disks - immediately, without any warning! What I would expect to see is a window saying something like:
--------------------------------------------------------------- There are 2 uninitialized DASD devices selected: /dev/disk/by-path/ccw-0.0.3627 /dev/disk/by-path/ccw-0.0.3727
To continue installation, the devices must be formatted. Formatting will remove any data on these devices. Format uninitialized DASD devices?
<Cancel> <Format> ----------------------------------------------------------------
Cancel will return to the Initial Destination spoke without formatting, OK (or Format?) button will start the formatting.
On Tue, Feb 25, 2014 at 04:49:34PM +0100, Jan Stodola wrote:
On 02/25/2014 03:57 PM, Samantha N. Bueno wrote:
On Tue, Feb 25, 2014 at 01:06:53PM +0100, Jan Stodola wrote:
Samantha, does it mean that anaconda will format all unformatted disks without asking? It looks so from the video and in my opinion this should not happen, anaconda should ask.
All disks, no; just the ones which are selected. Since one could have many disks which are unformatted, and since it is time-consuming to run dasdfmt especially as your number of disks grows, it was decided to not blanket dasdfmt all of them, especially since a user might not wish to use all of them.
I meant a bit different issue - I will describe better:
The main hub says "We won't touch your disks until you click 'Begin Installation'". In the Installation Destination spoke you selected 4 disks and clicked "Done". What happened: anaconda started formatting selected unformatted disks - immediately, without any warning! What I would expect to see is a window saying something like:
I don't like the idea of popping up yet another dialog box for someone to click a button so things can proceed. I am going to make sure that in the s390x installation guide, this behavior is mentioned specifically so that it won't (read: shouldn't) be a surprise.
I am even ok with amending the text on the main hub to say, "We won't touch your disks until you click 'Begin Installation', with the exception of DASDs requiring dasdfmt." (Only for s390x installs of course.)
To me it seems that if you have selected a disk that you want to use during installation, it's implied that you want to render it *usable* such that anaconda can actually utilize it.
Samantha
There are 2 uninitialized DASD devices selected: /dev/disk/by-path/ccw-0.0.3627 /dev/disk/by-path/ccw-0.0.3727
To continue installation, the devices must be formatted. Formatting will remove any data on these devices. Format uninitialized DASD devices?
<Cancel> <Format>
Cancel will return to the Initial Destination spoke without formatting, OK (or Format?) button will start the formatting. _______________________________________________ anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
On 02/25/2014 11:27 PM, Samantha N. Bueno wrote:
On Tue, Feb 25, 2014 at 04:49:34PM +0100, Jan Stodola wrote:
On 02/25/2014 03:57 PM, Samantha N. Bueno wrote:
On Tue, Feb 25, 2014 at 01:06:53PM +0100, Jan Stodola wrote:
Samantha, does it mean that anaconda will format all unformatted disks without asking? It looks so from the video and in my opinion this should not happen, anaconda should ask.
All disks, no; just the ones which are selected. Since one could have many disks which are unformatted, and since it is time-consuming to run dasdfmt especially as your number of disks grows, it was decided to not blanket dasdfmt all of them, especially since a user might not wish to use all of them.
I meant a bit different issue - I will describe better:
The main hub says "We won't touch your disks until you click 'Begin Installation'". In the Installation Destination spoke you selected 4 disks and clicked "Done". What happened: anaconda started formatting selected unformatted disks - immediately, without any warning! What I would expect to see is a window saying something like:
I don't like the idea of popping up yet another dialog box for someone to click a button so things can proceed. I am going to make sure that in the s390x installation guide, this behavior is mentioned specifically so that it won't (read: shouldn't) be a surprise.
It is not yet another dialog box, you can just use the dialog box you already implemented, but use the button "OK" to start the formatting. This way, user will have a chance to notice that something dangerous is going to happen with the disk(s). I think it is much better to ask the user rather than documenting the automatic formatting.
Also in RHEL-5 and RHEL-6, anaconda asks before formatting such disks.
I am even ok with amending the text on the main hub to say, "We won't touch your disks until you click 'Begin Installation', with the exception of DASDs requiring dasdfmt." (Only for s390x installs of course.)
To me it seems that if you have selected a disk that you want to use during installation, it's implied that you want to render it *usable* such that anaconda can actually utilize it.
Samantha
There are 2 uninitialized DASD devices selected: /dev/disk/by-path/ccw-0.0.3627 /dev/disk/by-path/ccw-0.0.3727
To continue installation, the devices must be formatted. Formatting will remove any data on these devices. Format uninitialized DASD devices?
<Cancel> <Format>
Cancel will return to the Initial Destination spoke without formatting, OK (or Format?) button will start the formatting. _______________________________________________ anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
This includes fixes suggested from the first review. The flow of this is also a bit different from when I first posted as well, thanks to discussions with dlehman and mizmo (read: simpler and less clunky). The text install is basically the same, but for an updated screencast of the graphical version here's a link: http://sbueno.fedorapeople.org/1001070-graphical-25feb.webm
jstodola mentioned the "zerombr" kickstart command last time; support for that is here; if "zerombr" is in a ks file and unformatted DASDs are specified, they automatically have dasdfmt run against them.
A few points that are sort of minor but which bug me, all graphical:
- The link on the dialog box back to the summary hub doesn't work, so I sort of force it myself in what's probably a less correct way. I'm not sure what format the URI should be, so if anyone knows, let me know.
I'm afraid the way you do it is the only working one.
- Again following a return to the summary hub, while letting dasdfmt run in the background, the UI sort of freezes up; I'm guessing I need some sort of decorator somewhere, maybe?
Does it freeze for the time DASDs being formated?
- I think my thread handling is jumbled and awkward in places (GUI storage), then again trying to cleanly work in the "zerombr" command was no picnic. I'd be greatly in favor of only allowing "zerombr" as valid if in text mode; the graphical code is hideous IMO.
I think it's not that bad even in the GUI code. :)
On Tue, Feb 25, 2014 at 01:19:05PM +0100, Vratislav Podzimek wrote:
On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
This includes fixes suggested from the first review. The flow of this is also a bit different from when I first posted as well, thanks to discussions with dlehman and mizmo (read: simpler and less clunky). The text install is basically the same, but for an updated screencast of the graphical version here's a link: http://sbueno.fedorapeople.org/1001070-graphical-25feb.webm
jstodola mentioned the "zerombr" kickstart command last time; support for that is here; if "zerombr" is in a ks file and unformatted DASDs are specified, they automatically have dasdfmt run against them.
A few points that are sort of minor but which bug me, all graphical:
- The link on the dialog box back to the summary hub doesn't work, so I sort of force it myself in what's probably a less correct way. I'm not sure what format the URI should be, so if anyone knows, let me know.
I'm afraid the way you do it is the only working one.
Eh, well that's fine. Just figured there's likely another way.
- Again following a return to the summary hub, while letting dasdfmt run in the background, the UI sort of freezes up; I'm guessing I need some sort of decorator somewhere, maybe?
Does it freeze for the time DASDs being formated?
Yes.
- I think my thread handling is jumbled and awkward in places (GUI storage), then again trying to cleanly work in the "zerombr" command was no picnic. I'd be greatly in favor of only allowing "zerombr" as valid if in text mode; the graphical code is hideous IMO.
I think it's not that bad even in the GUI code. :)
Ah you should've seen it before I'd trimmed things and squashed tmp patches. ;) The graphical "zerombr" stuff is still not perfect with the status notifications though so I'll likely re-post that once I figure it out.
Samantha
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
anaconda-patches@lists.fedorahosted.org
-
David Lehman -
Jan Stodola -
Samantha N. Bueno -
Vratislav Podzimek