From 52a82ea1e683139e5b71365899253ebd7ebf52b6 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Thu, 26 Aug 2021 17:39:06 +0200 Subject: [PATCH 1/4] [partition] Improve warning message in log --- src/modules/partition/core/PartUtils.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/partition/core/PartUtils.cpp b/src/modules/partition/core/PartUtils.cpp index 4beac0db8..8fe13f505 100644 --- a/src/modules/partition/core/PartUtils.cpp +++ b/src/modules/partition/core/PartUtils.cpp @@ -461,7 +461,7 @@ isEfiFilesystemSuitable(const Partition* candidate) { return true; } - cWarning() << "FAT32 filesystem is too small (" << size << "bytes)"; + cWarning() << "FAT32 filesystem for EFI is too small (" << size << "bytes)"; return false; #ifdef WITH_KPMCORE4API case FileSystem::Type::Fat12: From 7d0877080655171755039b2dcd17c1cfcfcc55eb Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 27 Aug 2021 17:26:50 +0200 Subject: [PATCH 2/4] [partition] Apply code style --- .../partition/core/PartitionLayout.cpp | 98 ++++++++++--------- 1 file changed, 50 insertions(+), 48 deletions(-) diff --git a/src/modules/partition/core/PartitionLayout.cpp b/src/modules/partition/core/PartitionLayout.cpp index 233f5117a..8ae904e92 100644 --- a/src/modules/partition/core/PartitionLayout.cpp +++ b/src/modules/partition/core/PartitionLayout.cpp @@ -138,61 +138,63 @@ PartitionLayout::init( FileSystem::Type defaultFsType, const QVariantList& confi } void -PartitionLayout::setDefaultFsType(FileSystem::Type defaultFsType) +PartitionLayout::setDefaultFsType( FileSystem::Type defaultFsType ) { using FileSystem = FileSystem::Type; switch ( defaultFsType ) { - case FileSystem::Unknown: - case FileSystem::Unformatted: - case FileSystem::Extended: - case FileSystem::LinuxSwap: - case FileSystem::Luks: - case FileSystem::Ocfs2: - case FileSystem::Lvm2_PV: - case FileSystem::Udf: - case FileSystem::Iso9660: + case FileSystem::Unknown: + case FileSystem::Unformatted: + case FileSystem::Extended: + case FileSystem::LinuxSwap: + case FileSystem::Luks: + case FileSystem::Ocfs2: + case FileSystem::Lvm2_PV: + case FileSystem::Udf: + case FileSystem::Iso9660: #ifdef WITH_KPMCORE4API - case FileSystem::Luks2: - case FileSystem::LinuxRaidMember: - case FileSystem::BitLocker: + case FileSystem::Luks2: + case FileSystem::LinuxRaidMember: + case FileSystem::BitLocker: #endif - // bad bad - cWarning() << "The selected default FS" << defaultFsType << "is not suitable." << "Using ext4 instead."; - defaultFsType = FileSystem::Ext4; - break; - case FileSystem::Ext2: - case FileSystem::Ext3: - case FileSystem::Ext4: - case FileSystem::Fat32: - case FileSystem::Ntfs: - case FileSystem::Reiser4: - case FileSystem::ReiserFS: - case FileSystem::Xfs: - case FileSystem::Jfs: - case FileSystem::Btrfs: - case FileSystem::Exfat: - case FileSystem::F2fs: - // ok - break; - case FileSystem::Fat16: - case FileSystem::Hfs: - case FileSystem::HfsPlus: - case FileSystem::Ufs: - case FileSystem::Hpfs: - case FileSystem::Zfs: - case FileSystem::Nilfs2: + // bad bad + cWarning() << "The selected default FS" << defaultFsType << "is not suitable." + << "Using ext4 instead."; + defaultFsType = FileSystem::Ext4; + break; + case FileSystem::Ext2: + case FileSystem::Ext3: + case FileSystem::Ext4: + case FileSystem::Fat32: + case FileSystem::Ntfs: + case FileSystem::Reiser4: + case FileSystem::ReiserFS: + case FileSystem::Xfs: + case FileSystem::Jfs: + case FileSystem::Btrfs: + case FileSystem::Exfat: + case FileSystem::F2fs: + // ok + break; + case FileSystem::Fat16: + case FileSystem::Hfs: + case FileSystem::HfsPlus: + case FileSystem::Ufs: + case FileSystem::Hpfs: + case FileSystem::Zfs: + case FileSystem::Nilfs2: #ifdef WITH_KPMCORE4API - case FileSystem::Fat12: - case FileSystem::Apfs: - case FileSystem::Minix: + case FileSystem::Fat12: + case FileSystem::Apfs: + case FileSystem::Minix: #endif - // weird - cWarning() << "The selected default FS" << defaultFsType << "is unusual, but not wrong."; - break; - default: - cWarning() << "The selected default FS" << defaultFsType << "is not known to Calamares." << "Using ext4 instead."; - defaultFsType = FileSystem::Ext4; + // weird + cWarning() << "The selected default FS" << defaultFsType << "is unusual, but not wrong."; + break; + default: + cWarning() << "The selected default FS" << defaultFsType << "is not known to Calamares." + << "Using ext4 instead."; + defaultFsType = FileSystem::Ext4; } m_defaultFsType = defaultFsType; @@ -278,7 +280,7 @@ PartitionLayout::createPartitions( Device* dev, } } - auto correctFS = [d=m_defaultFsType]( FileSystem::Type t ) { return t == FileSystem::Type::Unknown ? d : t; }; + auto correctFS = [d = m_defaultFsType]( FileSystem::Type t ) { return t == FileSystem::Type::Unknown ? d : t; }; // Create the partitions. currentSector = firstSector; From 6324fa3eb9ab1b5eab8b7eb5007368551c86bd4a Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 27 Aug 2021 17:27:26 +0200 Subject: [PATCH 3/4] [partition] Disentangle questions of suitability of ESP - split into size, type, flags so the warning message can be tailored to what is wrong. --- src/modules/partition/core/PartUtils.cpp | 56 ++++++++++++++++-------- src/modules/partition/core/PartUtils.h | 19 +++++++- 2 files changed, 54 insertions(+), 21 deletions(-) diff --git a/src/modules/partition/core/PartUtils.cpp b/src/modules/partition/core/PartUtils.cpp index 8fe13f505..54f971f4e 100644 --- a/src/modules/partition/core/PartUtils.cpp +++ b/src/modules/partition/core/PartUtils.cpp @@ -447,31 +447,40 @@ isEfiSystem() } bool -isEfiFilesystemSuitable(const Partition* candidate) +isEfiFilesystemSuitableType( const Partition* candidate ) { auto type = candidate->fileSystem().type(); + + switch ( type ) + { + case FileSystem::Type::Fat32: + return true; +#ifdef WITH_KPMCORE4API + case FileSystem::Type::Fat12: +#endif + case FileSystem::Type::Fat16: + cWarning() << "FAT12 and FAT16 are probably not supported by EFI"; + return false; + default: + cWarning() << "EFI boot partition must be FAT32"; + return false; + } +} + +bool +isEfiFilesystemSuitableSize( const Partition* candidate ) +{ auto size = candidate->capacity(); // bytes using CalamaresUtils::Units::operator""_MiB; - - switch( type ) + if ( size >= 300_MiB ) { - case FileSystem::Type::Fat32: - if ( size >= 300_MiB ) - { - return true; - } - cWarning() << "FAT32 filesystem for EFI is too small (" << size << "bytes)"; - return false; -#ifdef WITH_KPMCORE4API - case FileSystem::Type::Fat12: -#endif - case FileSystem::Type::Fat16: - cWarning() << "FAT12 and FAT16 are probably not supported by EFI"; - return false; - default: - cWarning() << "EFI boot partition must be FAT32"; - return false; + return true; + } + else + { + cWarning() << "Filesystem for EFI is too small (" << size << "bytes)"; + return false; } } @@ -508,6 +517,15 @@ isEfiBootable( const Partition* candidate ) #endif } +// TODO: this is configurable via the config file **already** +size_t +efiFilesystemMinimumSize() +{ + using CalamaresUtils::Units::operator""_MiB; + return 300_MiB; +} + + QString canonicalFilesystemName( const QString& fsName, FileSystem::Type* fsType ) { diff --git a/src/modules/partition/core/PartUtils.h b/src/modules/partition/core/PartUtils.h index 6bf223921..dd4efc867 100644 --- a/src/modules/partition/core/PartUtils.h +++ b/src/modules/partition/core/PartUtils.h @@ -84,9 +84,24 @@ bool isEfiSystem(); /** * @brief Is the @p partition suitable as an EFI boot partition? - * Checks for filesystem type (FAT32) and size (300MiB at least). + * Checks for filesystem type (FAT32). */ -bool isEfiFilesystemSuitable( const Partition* candidate ); +bool isEfiFilesystemSuitableType( const Partition* candidate ); + +/** + * @brief Is the @p partition suitable as an EFI boot partition? + * Checks for filesystem size (300MiB, see efiFilesystemMinimumSize). + */ +bool isEfiFilesystemSuitableSize( const Partition* candidate ); + +/** @brief Returns the minimum size of an EFI boot partition. + * + * This is determined as 300MiB, based on the FAT32 standard + * and EFI documentation (and not a little discussion in Calamares + * issues about what works, what is effective, and what is mandated + * by the standard and how all of those are different). + */ +size_t efiFilesystemMinimumSize(); /** * @brief Is the given @p partition bootable in EFI? Depending on From da49becac341adf3ae5a46818d2f9d41ca457da4 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 27 Aug 2021 17:28:07 +0200 Subject: [PATCH 4/4] [partition] Tailor warning message about ESP - tell the user all the things that are wrong with the (proposed) ESP; a missing one gets all the suggestions. --- src/modules/partition/PartitionViewStep.cpp | 74 +++++++++++++-------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/src/modules/partition/PartitionViewStep.cpp b/src/modules/partition/PartitionViewStep.cpp index d3d45bc8a..29e6524d9 100644 --- a/src/modules/partition/PartitionViewStep.cpp +++ b/src/modules/partition/PartitionViewStep.cpp @@ -434,50 +434,66 @@ PartitionViewStep::onLeave() { const QString espMountPoint = Calamares::JobQueue::instance()->globalStorage()->value( "efiSystemPartition" ).toString(); - const QString espFlagName = PartitionTable::flagName( #ifdef WITH_KPMCORE4API - PartitionTable::Flag::Boot + const auto espFlag = PartitionTable::Flag::Boot; #else - PartitionTable::FlagEsp + const auto espFlag = PartitionTable::FlagEsp; #endif - ); Partition* esp = m_core->findPartitionByMountPoint( espMountPoint ); QString message; QString description; - if ( !esp || ( esp && !PartUtils::isEfiFilesystemSuitable( esp ) ) ) + + Logger::Once o; + + const bool okType = esp && PartUtils::isEfiFilesystemSuitableType( esp ); + const bool okSize = esp && PartUtils::isEfiFilesystemSuitableSize( esp ); + const bool okFlag = esp && PartUtils::isEfiBootable( esp ); + + if ( !esp ) { message = tr( "No EFI system partition configured" ); - description = tr( "An EFI system partition is necessary to start %1." - "

" - "To configure an EFI system partition, go back and " - "select or create a FAT32 filesystem with the " - "%3 flag enabled and mount point " - "%2.

" - "You can continue without setting up an EFI system " - "partition but your system may fail to start." ) - .arg( branding->shortProductName() ) - .arg( espMountPoint, espFlagName ); } - else if ( esp && !PartUtils::isEfiBootable( esp ) ) + else if ( !(okType && okSize && okFlag ) ) { - message = tr( "EFI system partition flag not set" ); - description = tr( "An EFI system partition is necessary to start %1." - "

" - "A partition was configured with mount point " - "%2 but its %3 " - "flag is not set.
" - "To set the flag, go back and edit the partition." - "

" - "You can continue without setting the flag but your " - "system may fail to start." ) - .arg( branding->shortProductName() ) - .arg( espMountPoint, espFlagName ); + message = tr( "EFI system partition configured incorrectly" ); } + if ( !esp || !(okType&&okSize &&okFlag)) { + description = tr( "An EFI system partition is necessary to start %1." + "

" + "To configure an EFI system partition, go back and " + "select or create a suitable filesystem.").arg( branding->shortProductName() ); + } + if (!esp) { + cDebug() << o << "No ESP mounted"; + description.append(' '); + description.append(tr("The filesystem must be mounted on %1.").arg(espMountPoint)); + } + if (!okType) { + cDebug() << o << "ESP wrong type"; + description.append(' '); + description.append(tr("The filesystem must have type FAT32.")); + } + if (!okSize) { + cDebug() << o << "ESP too small"; + description.append(' '); + description.append(tr("The filesystem must be at least %1 MiB in size.").arg( PartUtils::efiFilesystemMinimumSize() )); + } + if (!okFlag) + { + cDebug() << o << "ESP missing flag"; + description.append(' '); + description.append(tr("The filesystem must have flag %1 set.").arg(PartitionTable::flagName( espFlag ))); + } + if (!description.isEmpty()) { + description.append( "

" ); + description.append( tr( + "You can continue without setting up an EFI system " + "partition but your system may fail to start." )); + } if ( !message.isEmpty() ) { - cWarning() << message; QMessageBox::warning( m_manualPartitionPage, message, description ); } }