From d630c2aadf392f6ab6c5ae49cafc6fa74664e9eb Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 25 May 2021 10:47:05 +0200 Subject: [PATCH 1/6] [partition] Introduce a check if the GPT-on-BIOS popup should be shown The check is bogus right now, and it still always warns; but if the `shouldWarnForGPTOnBIOS()` function is implemented, this will fix issue 1701. --- .../partition/gui/PartitionViewStep.cpp | 47 ++++++++++++------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/src/modules/partition/gui/PartitionViewStep.cpp b/src/modules/partition/gui/PartitionViewStep.cpp index 6755b64c6..b1fa851ed 100644 --- a/src/modules/partition/gui/PartitionViewStep.cpp +++ b/src/modules/partition/gui/PartitionViewStep.cpp @@ -395,6 +395,18 @@ PartitionViewStep::onActivate() } } +static bool +shouldWarnForGPTOnBIOS( const PartitionCoreModule* core ) +{ + if ( PartUtils::isEfiSystem() ) + { + return false; + } + + cDebug() << core->bootLoaderInstallPath(); + + return true; +} void PartitionViewStep::onLeave() @@ -462,24 +474,25 @@ PartitionViewStep::onLeave() { cDebug() << "device: BIOS"; - // TODO: this *always* warns, which might be annoying, so it'd be - // best to find a way to detect that bios_grub partition. - QString message = tr( "Option to use GPT on BIOS" ); - QString description = tr( "A GPT partition table is the best option for all " - "systems. This installer supports such a setup for " - "BIOS systems too." - "

" - "To configure a GPT partition table on BIOS, " - "(if not done so already) go back " - "and set the partition table to GPT, next create a 8 MB " - "unformatted partition with the " - "bios_grub flag enabled.

" - "An unformatted 8 MB partition is necessary " - "to start %1 on a BIOS system with GPT." ) - .arg( branding->shortProductName() ); + if ( shouldWarnForGPTOnBIOS( m_core ) ) + { + QString message = tr( "Option to use GPT on BIOS" ); + QString description = tr( "A GPT partition table is the best option for all " + "systems. This installer supports such a setup for " + "BIOS systems too." + "

" + "To configure a GPT partition table on BIOS, " + "(if not done so already) go back " + "and set the partition table to GPT, next create a 8 MB " + "unformatted partition with the " + "bios_grub flag enabled.

" + "An unformatted 8 MB partition is necessary " + "to start %1 on a BIOS system with GPT." ) + .arg( branding->shortProductName() ); - QMessageBox::information( m_manualPartitionPage, message, description ); + QMessageBox::information( m_manualPartitionPage, message, description ); + } } Partition* root_p = m_core->findPartitionByMountPoint( "/" ); @@ -593,7 +606,7 @@ PartitionViewStep::setConfigurationMap( const QVariantMap& configurationMap ) // because it could take a while. Then when it's done, we can set up the widgets // and remove the spinner. m_future = new QFutureWatcher< void >(); - connect( m_future, &QFutureWatcher< void >::finished, this, [this] { + connect( m_future, &QFutureWatcher< void >::finished, this, [ this ] { continueLoading(); this->m_future->deleteLater(); this->m_future = nullptr; From dabd895755e14b1926600f7743c013e235a1d64b Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 25 May 2021 12:33:12 +0200 Subject: [PATCH 2/6] [partition] Use type alias consistently --- src/modules/partition/core/BootLoaderModel.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/modules/partition/core/BootLoaderModel.h b/src/modules/partition/core/BootLoaderModel.h index 47e6ccb95..9b1a651e4 100644 --- a/src/modules/partition/core/BootLoaderModel.h +++ b/src/modules/partition/core/BootLoaderModel.h @@ -26,6 +26,8 @@ class BootLoaderModel : public QStandardItemModel { Q_OBJECT public: + using DeviceList = QList< Device* >; + enum { BootLoaderPathRole = Qt::UserRole + 1, @@ -39,14 +41,12 @@ public: * Init the model with the list of devices. Does *not* take ownership of the * devices. */ - void init( const QList< Device* >& devices ); + void init( const DeviceList& devices ); void update(); QVariant data( const QModelIndex& index, int role = Qt::DisplayRole ) const override; - using DeviceList = QList< Device* >; - private: DeviceList m_devices; mutable QMutex m_lock; From d0276fd25fde2717e81ac02c56454847ad175306 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 25 May 2021 13:05:45 +0200 Subject: [PATCH 3/6] [partition] Look up bootloader by name, method The bootloader model knows about both rows and devices, so we can look up both at once. The existing implementation as a non-member was rather sketchy and wasn't used except as support for restoreSelectedBootLoader(). --- .../partition/core/BootLoaderModel.cpp | 48 ++++++++++++------- src/modules/partition/core/BootLoaderModel.h | 15 +++--- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/src/modules/partition/core/BootLoaderModel.cpp b/src/modules/partition/core/BootLoaderModel.cpp index 08b0283b3..fd66c8514 100644 --- a/src/modules/partition/core/BootLoaderModel.cpp +++ b/src/modules/partition/core/BootLoaderModel.cpp @@ -18,6 +18,7 @@ // KPMcore #include +#include #include @@ -148,28 +149,39 @@ BootLoaderModel::data( const QModelIndex& index, int role ) const return QStandardItemModel::data( index, role ); } -namespace Calamares +std::pair< int, Device* > +BootLoaderModel::findBootLoader( const QString& path ) const { -int -findBootloader( const QAbstractItemModel* model, const QString& path ) -{ - for ( int i = 0; i < model->rowCount(); ++i ) + int r = 0; + for ( Device* d : m_devices ) { - const auto index = model->index( i, 0, QModelIndex() ); - if ( !index.isValid() ) + if ( d && d->deviceNode() == path ) { - continue; - } - QVariant var = model->data( index, BootLoaderModel::BootLoaderPathRole ); - if ( var.isValid() && var.toString() == path ) - { - return i; + return std::make_pair( r, d ); } + r++; } - return -1; + Partition* partition = KPMHelpers::findPartitionByMountPoint( m_devices, path ); + if ( partition ) + { + const QString partition_device_path = partition->deviceNode(); + r = 0; + for ( Device* d : m_devices ) + { + if ( d && d->deviceNode() == partition_device_path ) + { + return std::make_pair( r, d ); + } + r++; + } + } + return std::make_pair( -1, nullptr ); } + +namespace Calamares +{ void restoreSelectedBootLoader( QComboBox& combo, const QString& path ) { @@ -180,12 +192,16 @@ restoreSelectedBootLoader( QComboBox& combo, const QString& path ) return; } - int r = -1; if ( path.isEmpty() ) { + cDebug() << "No path to restore, choosing default"; combo.setCurrentIndex( 0 ); + return; } - else if ( ( r = findBootloader( model, path ) ) >= 0 ) + + const BootLoaderModel* bmodel = qobject_cast< const BootLoaderModel* >( model ); + int r = bmodel ? bmodel->findBootLoader( path ).first : -1; + if ( r >= 0 ) { combo.setCurrentIndex( r ); } diff --git a/src/modules/partition/core/BootLoaderModel.h b/src/modules/partition/core/BootLoaderModel.h index 9b1a651e4..e640d4d7c 100644 --- a/src/modules/partition/core/BootLoaderModel.h +++ b/src/modules/partition/core/BootLoaderModel.h @@ -47,6 +47,14 @@ public: QVariant data( const QModelIndex& index, int role = Qt::DisplayRole ) const override; + /** @brief Looks up a boot-loader by device-name @p path (e.g. /dev/sda) + * + * Returns a row number (index) in the model and a Device*: if there **is** a + * device for the given @p path, index will be in range of the model and + * Device* non-null. Returns (-1, nullptr) otherwise. + */ + std::pair< int, Device* > findBootLoader( const QString& path ) const; + private: DeviceList m_devices; mutable QMutex m_lock; @@ -57,13 +65,6 @@ private: namespace Calamares { -/** @brief Returns the row number of boot-loader @p path (e.g. /dev/sda) - * - * Assuming the @p model is a BootLoaderModel, will return a row number - * in the model. Returns -1 otherwise. - */ -int findBootloader( const QAbstractItemModel* model, const QString& path ); - /** @brief Tries to set @p path as selected item in @p combo * * Matches a boot-loader install path (e.g. /dev/sda) with a model From 43c172f54d596d3169e3ea4439c4a60a0d48921b Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 25 May 2021 13:23:28 +0200 Subject: [PATCH 4/6] [partition] Tighten up types Don't return the generic Abstract model for bootloader, but the subclass pointer, so that consumers can use the convenience API on the subclass. --- src/modules/partition/core/PartitionCoreModule.cpp | 2 +- src/modules/partition/core/PartitionCoreModule.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/modules/partition/core/PartitionCoreModule.cpp b/src/modules/partition/core/PartitionCoreModule.cpp index 058c10d18..27aceec60 100644 --- a/src/modules/partition/core/PartitionCoreModule.cpp +++ b/src/modules/partition/core/PartitionCoreModule.cpp @@ -340,7 +340,7 @@ PartitionCoreModule::deviceModel() const return m_deviceModel; } -QAbstractItemModel* +BootLoaderModel* PartitionCoreModule::bootLoaderModel() const { return m_bootLoaderModel; diff --git a/src/modules/partition/core/PartitionCoreModule.h b/src/modules/partition/core/PartitionCoreModule.h index 492348187..693569310 100644 --- a/src/modules/partition/core/PartitionCoreModule.h +++ b/src/modules/partition/core/PartitionCoreModule.h @@ -122,7 +122,7 @@ public: * The single BootLoaderModel instance belongs to the PCM. * @return the BootLoaderModel. */ - QAbstractItemModel* bootLoaderModel() const; + BootLoaderModel* bootLoaderModel() const; void createPartitionTable( Device* device, PartitionTable::TableType type ); From 252a88cb7fd60d2a526fc33f09608f644f9a3c28 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 25 May 2021 14:07:58 +0200 Subject: [PATCH 5/6] [partition] Check for suitable bios_grub partition. --- .../partition/gui/PartitionViewStep.cpp | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/src/modules/partition/gui/PartitionViewStep.cpp b/src/modules/partition/gui/PartitionViewStep.cpp index b1fa851ed..2e769576e 100644 --- a/src/modules/partition/gui/PartitionViewStep.cpp +++ b/src/modules/partition/gui/PartitionViewStep.cpp @@ -13,6 +13,7 @@ #include "gui/PartitionViewStep.h" +#include "core/BootLoaderModel.h" #include "core/Config.h" #include "core/DeviceModel.h" #include "core/KPMHelpers.h" @@ -36,6 +37,7 @@ #include "utils/NamedEnum.h" #include "utils/QtCompat.h" #include "utils/Retranslator.h" +#include "utils/Units.h" #include "utils/Variant.h" #include "widgets/WaitingWidget.h" @@ -403,8 +405,33 @@ shouldWarnForGPTOnBIOS( const PartitionCoreModule* core ) return false; } - cDebug() << core->bootLoaderInstallPath(); - + auto [ r, device ] = core->bootLoaderModel()->findBootLoader( core->bootLoaderInstallPath() ); + if ( device ) + { + auto* table = device->partitionTable(); + cDebug() << "Found device for bootloader" << device->deviceNode(); + if ( table && table->type() == PartitionTable::TableType::gpt ) + { + // So this is a BIOS system, and the bootloader will be installed on a GPT system + for ( const auto& partition : qAsConst( table->children() ) ) + { + using CalamaresUtils::Units::operator""_MiB; + if ( ( partition->activeFlags() & PartitionTable::Flag::BiosGrub ) + && ( partition->fileSystem().type() == FileSystem::Unformatted ) + && ( partition->capacity() >= 8_MiB ) ) + { + cDebug() << Logger::SubEntry << "Partition" << partition->partitionPath() + << "is a suitable bios_grub partition"; + return false; + } + } + } + cDebug() << Logger::SubEntry << "No suitable partition for bios_grub found"; + } + else + { + cDebug() << "Found no device for" << core->bootLoaderInstallPath(); + } return true; } From 01911beccc91273a020fa9f55092fbbf578da871 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 25 May 2021 14:16:28 +0200 Subject: [PATCH 6/6] [partition] Expand debugging output The partition path isn't set yet, so is probably 'empty'. Try logging the device, too. --- src/modules/partition/gui/PartitionViewStep.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/modules/partition/gui/PartitionViewStep.cpp b/src/modules/partition/gui/PartitionViewStep.cpp index 2e769576e..903b03380 100644 --- a/src/modules/partition/gui/PartitionViewStep.cpp +++ b/src/modules/partition/gui/PartitionViewStep.cpp @@ -420,7 +420,8 @@ shouldWarnForGPTOnBIOS( const PartitionCoreModule* core ) && ( partition->fileSystem().type() == FileSystem::Unformatted ) && ( partition->capacity() >= 8_MiB ) ) { - cDebug() << Logger::SubEntry << "Partition" << partition->partitionPath() + cDebug() << Logger::SubEntry << "Partition" << partition->devicePath() + << partition->partitionPath() << "is a suitable bios_grub partition"; return false; }