[PATCH 1/2] Revert "Restrict payload kernel versions to kernels in the payload (#1074358)"
This reverts commit 850063aa6a19f87a6ffd6009e00f15c3418d1111.
This idea turned out to have some issues, such as not handling kernel-core or kernel-debug packages. --- pyanaconda/bootloader.py | 7 +----- pyanaconda/packaging/__init__.py | 44 +++++++++++++++++++++++++++++-------- pyanaconda/packaging/dnfpayload.py | 7 ------ pyanaconda/packaging/livepayload.py | 34 ---------------------------- pyanaconda/packaging/tarpayload.py | 8 +++---- pyanaconda/packaging/yumpayload.py | 8 ------- 6 files changed, 39 insertions(+), 69 deletions(-)
diff --git a/pyanaconda/bootloader.py b/pyanaconda/bootloader.py index b017265..ac9ac5c 100644 --- a/pyanaconda/bootloader.py +++ b/pyanaconda/bootloader.py @@ -27,7 +27,6 @@ import re import struct import blivet from parted import PARTITION_BIOS_GRUB -from glob import glob
from pyanaconda import iutil from blivet.devicelibs import raid @@ -2388,11 +2387,7 @@ def writeBootLoader(storage, payload, instClass, ksdata): return
# get a list of installed kernel packages - # add whatever rescue kernels we can find to the end - kernel_versions = list(payload.kernelVersionList) - kernel_versions += glob(iutil.getSysroot() + "/boot/vmlinuz-*-rescue-*") - kernel_versions += glob(iutil.getSysroot() + "/boot/efi/EFI/%s/vmlinuz-*-rescue-*" % instClass.efi_dir) - + kernel_versions = payload.kernelVersionList + payload.rescueKernelList if not kernel_versions: log.warning("no kernel was installed -- boot loader config unchanged") return diff --git a/pyanaconda/packaging/__init__.py b/pyanaconda/packaging/__init__.py index 2438b6f..64985ac 100644 --- a/pyanaconda/packaging/__init__.py +++ b/pyanaconda/packaging/__init__.py @@ -72,15 +72,8 @@ from pyanaconda.product import productName, productVersion import urlgrabber urlgrabber.grabber.default_grabber.opts.user_agent = "%s (anaconda)/%s" %(productName, productVersion)
-from distutils.version import LooseVersion - REPO_NOT_SET = False
-def versionCmp(v1, v2): - """ Compare two version number strings. """ - - return LooseVersion(v1).__cmp__(LooseVersion(v2)) - ### ### ERROR HANDLING ### @@ -135,6 +128,8 @@ class Payload(object): self.data = data self.storage = None self.instclass = None + self._kernelVersionList = [] + self._rescueVersionList = [] self.txID = None
def setup(self, storage, instClass): @@ -307,6 +302,29 @@ class Payload(object): self.data.packages.excludedGroupList.append(grp)
### + ### METHODS FOR WORKING WITH PACKAGES + ### + def _updateKernelVersionList(self): + try: + import yum + except ImportError: + cmpfunc = cmp + else: + cmpfunc = yum.rpmUtils.miscutils.compareVerOnly + + files = glob(iutil.getSysroot() + "/boot/vmlinuz-*") + files.extend(glob(iutil.getSysroot() + "/boot/efi/EFI/%s/vmlinuz-*" % self.instclass.efi_dir)) + + versions = sorted((f.split("/")[-1][8:] for f in files if os.path.isfile(f)), cmp=cmpfunc) + log.debug("kernel versions: %s", versions) + + # Store regular and rescue kernels separately + self._kernelVersionList = ( + [v for v in versions if "-rescue-" not in v], + [v for v in versions if "-rescue-" in v] + ) + + ### ### METHODS FOR QUERYING STATE ### @property @@ -316,8 +334,16 @@ class Payload(object):
@property def kernelVersionList(self): - """ An iterable of the kernel versions installed by the payload. """ - raise NotImplementedError() + if not self._kernelVersionList: + self._updateKernelVersionList() + + return self._kernelVersionList[0] + + @property + def rescueKernelList(self): + # do re-scan if looking for rescue kernel + self._updateKernelVersionList() + return self._kernelVersionList[1]
## ## METHODS FOR TREE VERIFICATION diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py index 6cea430..0fd3ac3 100644 --- a/pyanaconda/packaging/dnfpayload.py +++ b/pyanaconda/packaging/dnfpayload.py @@ -820,10 +820,3 @@ class DNFPayload(packaging.PackagePayload): log.error(e)
super(DNFPayload, self).postInstall() - - @property - def kernelVersionList(self): - return ('%s-%s.%s' % (p.version, p.release, p.arch) for p in - sorted(self._base.sack.query().filter(name='kernel'), - cmp=dnf.rpm.miscutils.compareEVR, - key=lambda p: (p.epoch, p.version, p.release))) diff --git a/pyanaconda/packaging/livepayload.py b/pyanaconda/packaging/livepayload.py index 615bb61..17c1bac 100644 --- a/pyanaconda/packaging/livepayload.py +++ b/pyanaconda/packaging/livepayload.py @@ -56,7 +56,6 @@ from blivet.size import Size import blivet.util from pyanaconda.threads import threadMgr, AnacondaThread from pyanaconda.i18n import _ -from pyanaconda.packaging import versionCmp
class LiveImagePayload(ImagePayload): """ A LivePayload copies the source image onto the target system. """ @@ -68,8 +67,6 @@ class LiveImagePayload(ImagePayload): self.pct_lock = None self.source_size = 1
- self._kernelVersionList = [] - def setup(self, storage, instClass): super(LiveImagePayload, self).setup(storage, instClass)
@@ -83,9 +80,6 @@ class LiveImagePayload(ImagePayload): if rc != 0: raise PayloadInstallError("Failed to mount the install tree")
- # Grab the kernel version list now so it's available after umount - self._updateKernelVersionList() - source = iutil.eintr_retry_call(os.statvfs, INSTALL_TREE) self.source_size = source.f_frsize * (source.f_blocks - source.f_bfree)
@@ -186,17 +180,6 @@ class LiveImagePayload(ImagePayload): def spaceRequired(self): return Size(iutil.getDirSize("/")*1024)
- def _updateKernelVersionList(self): - files = glob.glob(INSTALL_TREE + "/boot/vmlinuz-*") - files.extend(glob.glob(INSTALL_TREE + "/boot/efi/EFI/%s/vmlinuz-*" % self.instclass.efi_dir)) - - self._kernelVersionList = sorted((f.split("/")[-1][8:] for f in files - if os.path.isfile(f) and "-rescue-" not in f), cmp=versionCmp) - - @property - def kernelVersionList(self): - return self._kernelVersionList - class URLGrabberProgress(object): """ Provide methods for urlgrabber progress.""" def start(self, filename, url, basename, size, text): @@ -405,7 +388,6 @@ class LiveImageKSPayload(LiveImagePayload):
# Nothing more to mount if not os.path.exists(INSTALL_TREE+"/LiveOS"): - self._updateKernelVersionList() return
# Mount the first .img in the directory on INSTALL_TREE @@ -433,8 +415,6 @@ class LiveImageKSPayload(LiveImagePayload): if errorHandler.cb(exn) == ERROR_RAISE: raise exn
- self._updateKernelVersionList() - source = iutil.eintr_retry_call(os.statvfs, INSTALL_TREE) self.source_size = source.f_frsize * (source.f_blocks - source.f_bfree)
@@ -513,17 +493,3 @@ class LiveImageKSPayload(LiveImagePayload): return Size(self._min_size) else: return Size(1024*1024*1024) - - @property - def kernelVersionList(self): - # If it doesn't look like a tarfile use the super's kernelVersionList - if not self.is_tarfile: - return super(LiveImageKSPayload, self).kernelVersionList - - import tarfile - with tarfile.open(self.image_path) as archive: - names = archive.getnames() - - # Strip out vmlinuz- from the names - return sorted((n.split("/")[-1][8:] for n in names if "boot/vmlinuz-" in n), - cmp=versionCmp) diff --git a/pyanaconda/packaging/tarpayload.py b/pyanaconda/packaging/tarpayload.py index 805515d..b6328e0 100644 --- a/pyanaconda/packaging/tarpayload.py +++ b/pyanaconda/packaging/tarpayload.py @@ -36,7 +36,7 @@ except ImportError: log.error("import of tarfile failed") tarfile = None
-from pyanaconda.packaging import ArchivePayload, PayloadError, versionCmp +from pyanaconda.packaging import ArchivePayload, PayloadError from pyanaconda import iutil
# TarPayload is not yet fully implemented @@ -73,10 +73,8 @@ class TarPayload(ArchivePayload): @property def kernelVersionList(self): names = self.archive.getnames() - - # Strip out vmlinuz- from the names - return sorted((n.split("/")[-1][8:] for n in names if "boot/vmlinuz-" in n), - cmp=versionCmp) + kernels = [n for n in names if "boot/vmlinuz-" in n] + return kernels
def install(self): try: diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py index 73caa49..742edad 100644 --- a/pyanaconda/packaging/yumpayload.py +++ b/pyanaconda/packaging/yumpayload.py @@ -988,14 +988,6 @@ reposdir=%s else: return environment.groups
- @property - def kernelVersionList(self): - with _yum_lock: - return ("%s-%s.%s" % (p.version, p.release, p.arch) for p in - sorted(self._yum.pkgSack.searchNames(['kernel']), - cmp=yum.rpmUtils.miscutils.compareEVR, - key=lambda p: (p.epoch, p.version, p.release))) - ### ### METHODS FOR WORKING WITH GROUPS ###
Disallow the reuse of /boot and /var and some other things. --- pyanaconda/ui/gui/spokes/custom.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py index 2866011..fc1e7ad 100644 --- a/pyanaconda/ui/gui/spokes/custom.py +++ b/pyanaconda/ui/gui/spokes/custom.py @@ -654,8 +654,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker): error = _("%s cannot be encrypted") % mountpoint elif encrypted and new_fs_type in PARTITION_ONLY_FORMAT_TYPES: error = _("%s cannot be encrypted") % new_fs_type - elif mountpoint == "/" and device.format.exists and not reformat: - error = _("You must create a new file system on the root device.") + elif not reformat and not self.storage.formatByDefault(device): + error = _("You must create a new file system on the %s device.") % mountpoint
if not error and \ (raid_level is not None or requiresRaidSelection(device_type)) and \
On 01/21/2015 05:58 PM, David Shea wrote:
Disallow the reuse of /boot and /var and some other things.
pyanaconda/ui/gui/spokes/custom.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py index 2866011..fc1e7ad 100644 --- a/pyanaconda/ui/gui/spokes/custom.py +++ b/pyanaconda/ui/gui/spokes/custom.py @@ -654,8 +654,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker): error = _("%s cannot be encrypted") % mountpoint elif encrypted and new_fs_type in PARTITION_ONLY_FORMAT_TYPES: error = _("%s cannot be encrypted") % new_fs_type
elif mountpoint == "/" and device.format.exists and not reformat:error = _("You must create a new file system on the root device.")
elif not reformat and not self.storage.formatByDefault(device):error = _("You must create a new file system on the %s device.") % mountpoint if not error and \ (raid_level is not None or requiresRaidSelection(device_type)) and \
Just in case this doesn't pan out for whatever reason, the other idea discussed on IRC was to query the rpm database for what kernels were installed by a package payload, so that would be to implement PackagePayload.kernelVersionList as something like
ts = rpm.TransactionSet(iutil.getSysroot()) mi = ts.dbMatch('providename', 'kernel') for hdr in mi: (do something with hdr.filenames)
On Wed, 2015-01-21 at 17:58 -0500, David Shea wrote:
Disallow the reuse of /boot and /var and some other things.
pyanaconda/ui/gui/spokes/custom.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py index 2866011..fc1e7ad 100644 --- a/pyanaconda/ui/gui/spokes/custom.py +++ b/pyanaconda/ui/gui/spokes/custom.py @@ -654,8 +654,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker): error = _("%s cannot be encrypted") % mountpoint elif encrypted and new_fs_type in PARTITION_ONLY_FORMAT_TYPES: error = _("%s cannot be encrypted") % new_fs_type
elif mountpoint == "/" and device.format.exists and notreformat:
error = _("You must create a new file system on theroot device.")
elif not reformat and notself.storage.formatByDefault(device):
error = _("You must create a new file system on the %sdevice.") % mountpoint
Isn't that the wrong way around? By my read, formatByDefault returns true if the device must be reformatted, so it should be:
elif not reformat and self.storage.formatByDefault(device):
you also seem to have dropped the 'and device.format.exists' condition - is that not needed?
btw, wouldn't it be somewhat nicer UI to grey out the format checkbox for mount points that must be reformatted and add a little note or tooltip explaining the situation, rather than letting people create an invalid configuration then raising an error?
On 01/22/2015 04:16 PM, Adam Williamson wrote:
On Wed, 2015-01-21 at 17:58 -0500, David Shea wrote:
Disallow the reuse of /boot and /var and some other things.
pyanaconda/ui/gui/spokes/custom.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py index 2866011..fc1e7ad 100644 --- a/pyanaconda/ui/gui/spokes/custom.py +++ b/pyanaconda/ui/gui/spokes/custom.py @@ -654,8 +654,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker): error = _("%s cannot be encrypted") % mountpoint elif encrypted and new_fs_type in PARTITION_ONLY_FORMAT_TYPES: error = _("%s cannot be encrypted") % new_fs_type
elif mountpoint == "/" and device.format.exists and notreformat:
error = _("You must create a new file system on theroot device.")
elif not reformat and notself.storage.formatByDefault(device):
error = _("You must create a new file system on the %sdevice.") % mountpoint
Isn't that the wrong way around? By my read, formatByDefault returns true if the device must be reformatted, so it should be:
elif not reformat and self.storage.formatByDefault(device):
you also seem to have dropped the 'and device.format.exists' condition
- is that not needed?
Yeah. Additional fun facts: formatByDefault does not check /, and it was always returning false anyway since format.mountpoint isn't set here. So this needs some work.
btw, wouldn't it be somewhat nicer UI to grey out the format checkbox for mount points that must be reformatted and add a little note or tooltip explaining the situation, rather than letting people create an invalid configuration then raising an error?
Not doing any UI updates until the user hits the update button is consistent with the rest of how custom storage works, and given how many fiddlable bits there are on the mountpoint input I thought we had settled on that being the best policy.
On 01/22/2015 04:48 PM, David Shea wrote:
formatByDefault does not check /
That's a lie, it's just hardcoded into a conditional instead of listed in that list
, and it was always returning false anyway since format.mountpoint isn't set here. So this needs some work.
but that's still true.
On Thu, 2015-01-22 at 16:54 -0500, David Shea wrote:
On 01/22/2015 04:48 PM, David Shea wrote:
formatByDefault does not check /
That's a lie, it's just hardcoded into a conditional instead of listed in that list
Right. The reason (I figure) is that, for obvious reasons, we don't want to do the 'startswith' check for / , because it'd match everything. Hence the split, so we can do the 'startswith' check for things-other-than-root, and only do an exact match check for root.
, and it was always returning false anyway since format.mountpoint isn't set here. So this needs some work.
but that's still true.
Heh, didn't check it that far, I just eyeballed it :)
These both look good.
anaconda-patches@lists.fedorahosted.org
-
Adam Williamson -
Brian C. Lane -
David Shea