From 356e13ae93db83c83e77934387f75520a4470c2c Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 3 Nov 2020 23:46:07 +0100 Subject: [PATCH 1/8] [partition] Improve logging readability --- src/modules/partition/gui/EditExistingPartitionDialog.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/modules/partition/gui/EditExistingPartitionDialog.cpp b/src/modules/partition/gui/EditExistingPartitionDialog.cpp index 1e66c539c..39900430b 100644 --- a/src/modules/partition/gui/EditExistingPartitionDialog.cpp +++ b/src/modules/partition/gui/EditExistingPartitionDialog.cpp @@ -136,8 +136,8 @@ EditExistingPartitionDialog::applyChanges( PartitionCoreModule* core ) bool partResizedMoved = newFirstSector != m_partition->firstSector() || newLastSector != m_partition->lastSector(); cDebug() << "old boundaries:" << m_partition->firstSector() << m_partition->lastSector() << m_partition->length(); - cDebug() << "new boundaries:" << newFirstSector << newLastSector; - cDebug() << "dirty status:" << m_partitionSizeController->isDirty(); + cDebug() << Logger::SubEntry << "new boundaries:" << newFirstSector << newLastSector; + cDebug() << Logger::SubEntry << "dirty status:" << m_partitionSizeController->isDirty(); FileSystem::Type fsType = FileSystem::Unknown; if ( m_ui->formatRadioButton->isChecked() ) From 87c77d9807f428992e854d48e82b1fa53bee6266 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 4 Nov 2020 00:14:05 +0100 Subject: [PATCH 2/8] [partition] When flags are explicitly invalid, return early --- src/modules/partition/core/PartitionInfo.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/modules/partition/core/PartitionInfo.cpp b/src/modules/partition/core/PartitionInfo.cpp index 87aa03b66..9ab59ecf3 100644 --- a/src/modules/partition/core/PartitionInfo.cpp +++ b/src/modules/partition/core/PartitionInfo.cpp @@ -52,6 +52,10 @@ PartitionTable::Flags flags( const Partition* partition ) { auto v = partition->property( FLAGS_PROPERTY ); + if ( !v.isValid() ) + { + return partition->activeFlags(); + } if ( v.type() == QVariant::Int ) { return static_cast< PartitionTable::Flags >( v.toInt() ); From 63964de4bdc4ab595570454c4681a162715ba840 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 4 Nov 2020 00:32:21 +0100 Subject: [PATCH 3/8] [partition] Explain underlying type for flags variant --- src/modules/partition/core/PartitionInfo.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/modules/partition/core/PartitionInfo.cpp b/src/modules/partition/core/PartitionInfo.cpp index 9ab59ecf3..c4d239db8 100644 --- a/src/modules/partition/core/PartitionInfo.cpp +++ b/src/modules/partition/core/PartitionInfo.cpp @@ -56,7 +56,11 @@ flags( const Partition* partition ) { return partition->activeFlags(); } - if ( v.type() == QVariant::Int ) + // The underlying type of PartitionTable::Flags can be int or uint + // (see qflags.h) and so setting those flags can create a QVariant + // of those types; we don't just want to check QVariant::canConvert() + // here because that will also accept QByteArray and some other things. + if ( v.type() == QVariant::Int || v.type() == QVariant::UInt ) { return static_cast< PartitionTable::Flags >( v.toInt() ); } From 949e33f1e8d7573c5691a2c907f62525e4118616 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 4 Nov 2020 00:43:32 +0100 Subject: [PATCH 4/8] [partition] Massage logging while checking for EFI boot --- src/modules/partition/core/PartUtils.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/modules/partition/core/PartUtils.cpp b/src/modules/partition/core/PartUtils.cpp index 07ab5acc1..bbf63e79d 100644 --- a/src/modules/partition/core/PartUtils.cpp +++ b/src/modules/partition/core/PartUtils.cpp @@ -439,10 +439,8 @@ isEfiSystem() bool isEfiBootable( const Partition* candidate ) { - cDebug() << "Check EFI bootable" << convenienceName( candidate ) << candidate->devicePath(); - cDebug() << Logger::SubEntry << "flags" << candidate->activeFlags(); - auto flags = PartitionInfo::flags( candidate ); + cDebug() << "Check EFI bootable" << convenienceName( candidate ) << candidate->devicePath() << "flags" << flags; /* If bit 17 is set, old-style Esp flag, it's OK */ if ( flags.testFlag( KPM_PARTITION_FLAG_ESP ) ) @@ -455,19 +453,28 @@ isEfiBootable( const Partition* candidate ) while ( root && !root->isRoot() ) { root = root->parent(); - cDebug() << Logger::SubEntry << "moved towards root" << Logger::Pointer( root ); } // Strange case: no root found, no partition table node? if ( !root ) { + cWarning() << "No root of partition table found."; return false; } const PartitionTable* table = dynamic_cast< const PartitionTable* >( root ); - cDebug() << Logger::SubEntry << "partition table" << Logger::Pointer( table ) << "type" - << ( table ? table->type() : PartitionTable::TableType::unknownTableType ); - return table && ( table->type() == PartitionTable::TableType::gpt ) && flags.testFlag( KPM_PARTITION_FLAG( Boot ) ); + if ( !table ) + { + cWarning() << "Root of partition table is not a PartitionTable object"; + return false; + } + if ( table->type() == PartitionTable::TableType::gpt ) + { + const auto bootFlag = KPM_PARTITION_FLAG( Boot ); + cDebug() << Logger::SubEntry << "GPT table" << flags << "boot?" << bootFlag << flags.testFlag( bootFlag ); + return flags.testFlag( bootFlag ); + } + return false; } QString From 85bb8c27b3437a6ecd63d002071e3760c17091b8 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 4 Nov 2020 00:58:11 +0100 Subject: [PATCH 5/8] [partition] Simplify flags calculations - factor out the flags-we-want from the flags-we-already-have - the use of ->activeFlags() meant that the state on *disk* was being compared with the flags-we-want; if a partition was re-edited, then you couldn't change the flags back to the state-on-disk (eg. enable a flag, then change your mind and disable it). - set the flags before refreshing the partition, because the refresh checks for EFI bootability and that needs the new flags, not the old ones. --- .../gui/EditExistingPartitionDialog.cpp | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/modules/partition/gui/EditExistingPartitionDialog.cpp b/src/modules/partition/gui/EditExistingPartitionDialog.cpp index 39900430b..3de6e0c4c 100644 --- a/src/modules/partition/gui/EditExistingPartitionDialog.cpp +++ b/src/modules/partition/gui/EditExistingPartitionDialog.cpp @@ -147,6 +147,9 @@ EditExistingPartitionDialog::applyChanges( PartitionCoreModule* core ) : FileSystem::typeForName( m_ui->fileSystemComboBox->currentText() ); } + const auto resultFlags = newFlags(); + const auto currentFlags = PartitionInfo::flags( m_partition ); + if ( partResizedMoved ) { if ( m_ui->formatRadioButton->isChecked() ) @@ -157,20 +160,20 @@ EditExistingPartitionDialog::applyChanges( PartitionCoreModule* core ) fsType, newFirstSector, newLastSector, - newFlags() ); + resultFlags ); PartitionInfo::setMountPoint( newPartition, PartitionInfo::mountPoint( m_partition ) ); PartitionInfo::setFormat( newPartition, true ); core->deletePartition( m_device, m_partition ); core->createPartition( m_device, newPartition ); - core->setPartitionFlags( m_device, newPartition, newFlags() ); + core->setPartitionFlags( m_device, newPartition, resultFlags ); } else { core->resizePartition( m_device, m_partition, newFirstSector, newLastSector ); - if ( m_partition->activeFlags() != newFlags() ) + if ( currentFlags != resultFlags ) { - core->setPartitionFlags( m_device, m_partition, newFlags() ); + core->setPartitionFlags( m_device, m_partition, resultFlags ); } } } @@ -183,9 +186,9 @@ EditExistingPartitionDialog::applyChanges( PartitionCoreModule* core ) if ( m_partition->fileSystem().type() == fsType ) { core->formatPartition( m_device, m_partition ); - if ( m_partition->activeFlags() != newFlags() ) + if ( currentFlags != resultFlags ) { - core->setPartitionFlags( m_device, m_partition, newFlags() ); + core->setPartitionFlags( m_device, m_partition, resultFlags ); } } else // otherwise, we delete and recreate the partition with new fs type @@ -196,22 +199,22 @@ EditExistingPartitionDialog::applyChanges( PartitionCoreModule* core ) fsType, m_partition->firstSector(), m_partition->lastSector(), - newFlags() ); + resultFlags ); PartitionInfo::setMountPoint( newPartition, PartitionInfo::mountPoint( m_partition ) ); PartitionInfo::setFormat( newPartition, true ); core->deletePartition( m_device, m_partition ); core->createPartition( m_device, newPartition ); - core->setPartitionFlags( m_device, newPartition, newFlags() ); + core->setPartitionFlags( m_device, newPartition, resultFlags ); } } else { - core->refreshPartition( m_device, m_partition ); - if ( m_partition->activeFlags() != newFlags() ) + if ( currentFlags != resultFlags ) { - core->setPartitionFlags( m_device, m_partition, newFlags() ); + core->setPartitionFlags( m_device, m_partition, resultFlags ); } + core->refreshPartition( m_device, m_partition ); } } } From 15ace5202df15390cea1d9af039dc753894af8b8 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 4 Nov 2020 01:16:36 +0100 Subject: [PATCH 6/8] [partition] Simplify EFI-flags checking with KPMCore 4 --- src/modules/partition/core/PartUtils.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/modules/partition/core/PartUtils.cpp b/src/modules/partition/core/PartUtils.cpp index bbf63e79d..5cd68c5bf 100644 --- a/src/modules/partition/core/PartUtils.cpp +++ b/src/modules/partition/core/PartUtils.cpp @@ -439,9 +439,13 @@ isEfiSystem() bool isEfiBootable( const Partition* candidate ) { - auto flags = PartitionInfo::flags( candidate ); - cDebug() << "Check EFI bootable" << convenienceName( candidate ) << candidate->devicePath() << "flags" << flags; + const auto flags = PartitionInfo::flags( candidate ); + // TODO: with KPMCore 4, this comment is wrong: the flags + // are remapped, and the ESP flag is the same as Boot. +#if defined( WITH_KPMCORE4API ) + return flags.testFlag( KPM_PARTITION_FLAG_ESP ); +#else /* If bit 17 is set, old-style Esp flag, it's OK */ if ( flags.testFlag( KPM_PARTITION_FLAG_ESP ) ) { @@ -471,10 +475,10 @@ isEfiBootable( const Partition* candidate ) if ( table->type() == PartitionTable::TableType::gpt ) { const auto bootFlag = KPM_PARTITION_FLAG( Boot ); - cDebug() << Logger::SubEntry << "GPT table" << flags << "boot?" << bootFlag << flags.testFlag( bootFlag ); return flags.testFlag( bootFlag ); } return false; +#endif } QString From 0f38ee624ec77a4c8091bb53385a6fccf337d785 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 4 Nov 2020 09:59:35 +0100 Subject: [PATCH 7/8] [partition] static-assert that our shortcut makes sense --- src/modules/partition/core/PartUtils.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/modules/partition/core/PartUtils.cpp b/src/modules/partition/core/PartUtils.cpp index 5cd68c5bf..92bcbace0 100644 --- a/src/modules/partition/core/PartUtils.cpp +++ b/src/modules/partition/core/PartUtils.cpp @@ -444,6 +444,7 @@ isEfiBootable( const Partition* candidate ) // TODO: with KPMCore 4, this comment is wrong: the flags // are remapped, and the ESP flag is the same as Boot. #if defined( WITH_KPMCORE4API ) + static_assert( KPM_PARTITION_FLAG_ESP == KPM_PARTITION_FLAG( Boot ), "KPMCore API enum changed" ); return flags.testFlag( KPM_PARTITION_FLAG_ESP ); #else /* If bit 17 is set, old-style Esp flag, it's OK */ From 2c297a068feb1cd60a1c3ea46f04803601c57b6c Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 4 Nov 2020 12:32:51 +0100 Subject: [PATCH 8/8] [partition] Log when an EFI problem has been solved --- src/modules/partition/core/PartitionCoreModule.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/modules/partition/core/PartitionCoreModule.cpp b/src/modules/partition/core/PartitionCoreModule.cpp index 14ce48b88..254d007c1 100644 --- a/src/modules/partition/core/PartitionCoreModule.cpp +++ b/src/modules/partition/core/PartitionCoreModule.cpp @@ -716,6 +716,8 @@ PartitionCoreModule::updateIsDirty() void PartitionCoreModule::scanForEfiSystemPartitions() { + const bool wasEmpty = m_efiSystemPartitions.isEmpty(); + m_efiSystemPartitions.clear(); QList< Device* > devices; @@ -732,6 +734,11 @@ PartitionCoreModule::scanForEfiSystemPartitions() { cWarning() << "system is EFI but no EFI system partitions found."; } + else if ( wasEmpty ) + { + // But it isn't empty anymore, so whatever problem has been solved + cDebug() << "system is EFI and new EFI system partition has been found."; + } m_efiSystemPartitions = efiSystemPartitions; }