From fe662345bd1ac47a8dbd4833b965f0ac677444a7 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 3 Sep 2018 10:57:20 -0400 Subject: [PATCH] [partition] Extra helper classes for doing reset and refresh - The ResetHelper only finalized changes to the module on destruction, but calls to refresh() assumed it was already done. This leads to crashes when refresh() uses an intermediate state of the model. Introduce extra helpers, and rename refresh() to avoid calling the old implementation from any code. The new helper just creates and destroys a ResetHelper, before creating and destroying an object that calls the new refreshAfterModelChange(). FIXES #1019 --- .../partition/core/PartitionCoreModule.cpp | 73 ++++++++++++------- .../partition/core/PartitionCoreModule.h | 21 +++++- 2 files changed, 65 insertions(+), 29 deletions(-) diff --git a/src/modules/partition/core/PartitionCoreModule.cpp b/src/modules/partition/core/PartitionCoreModule.cpp index f6eba9288..542b7e329 100644 --- a/src/modules/partition/core/PartitionCoreModule.cpp +++ b/src/modules/partition/core/PartitionCoreModule.cpp @@ -64,6 +64,37 @@ #include #include + +PartitionCoreModule::RefreshHelper::RefreshHelper(PartitionCoreModule* module) + : m_module( module ) +{ +} + +PartitionCoreModule::RefreshHelper::~RefreshHelper() +{ + m_module->refreshAfterModelChange(); +} + +class OperationHelper +{ +public: + OperationHelper( PartitionModel* model, PartitionCoreModule* core ) + : m_modelHelper( model ) + , m_coreHelper( core ) + { + } + + OperationHelper( const OperationHelper& ) = delete; + OperationHelper& operator=( const OperationHelper& ) = delete; + +private: + // Keep these in order: first the model needs to finish, + // then refresh is called. + PartitionModel::ResetHelper m_modelHelper; + PartitionCoreModule::RefreshHelper m_coreHelper; +} ; + + //- DeviceInfo --------------------------------------------- PartitionCoreModule::DeviceInfo::DeviceInfo( Device* _device ) : device( _device ) @@ -242,13 +273,11 @@ PartitionCoreModule::createPartitionTable( Device* device, PartitionTable::Table // keep previous changes info->forgetChanges(); - PartitionModel::ResetHelper helper( partitionModelForDevice( device ) ); + OperationHelper helper( partitionModelForDevice( device ), this ); CreatePartitionTableJob* job = new CreatePartitionTableJob( device, type ); job->updatePreview(); info->jobs << Calamares::job_ptr( job ); } - - refresh(); } void @@ -259,7 +288,7 @@ PartitionCoreModule::createPartition( Device* device, auto deviceInfo = infoForDevice( device ); Q_ASSERT( deviceInfo ); - PartitionModel::ResetHelper helper( partitionModelForDevice( device ) ); + OperationHelper helper( partitionModelForDevice( device ), this ); CreatePartitionJob* job = new CreatePartitionJob( device, partition ); job->updatePreview(); @@ -271,8 +300,6 @@ PartitionCoreModule::createPartition( Device* device, deviceInfo->jobs << Calamares::job_ptr( fJob ); PartitionInfo::setFlags( partition, flags ); } - - refresh(); } void @@ -301,7 +328,7 @@ PartitionCoreModule::createVolumeGroup( QString &vgName, m_deviceInfos << deviceInfo; deviceInfo->jobs << Calamares::job_ptr( job ); - refresh(); + refreshAfterModelChange(); } void @@ -314,7 +341,7 @@ PartitionCoreModule::resizeVolumeGroup( LvmDevice *device, QVector< const Partit deviceInfo->jobs << Calamares::job_ptr( job ); - refresh(); + refreshAfterModelChange(); } void @@ -330,7 +357,7 @@ PartitionCoreModule::deactivateVolumeGroup( LvmDevice *device ) // DeactivateVolumeGroupJob needs to be immediately called job->exec(); - refresh(); + refreshAfterModelChange(); } void @@ -343,7 +370,7 @@ PartitionCoreModule::removeVolumeGroup( LvmDevice *device ) deviceInfo->jobs << Calamares::job_ptr( job ); - refresh(); + refreshAfterModelChange(); } void @@ -352,7 +379,7 @@ PartitionCoreModule::deletePartition( Device* device, Partition* partition ) auto deviceInfo = infoForDevice( device ); Q_ASSERT( deviceInfo ); - PartitionModel::ResetHelper helper( partitionModelForDevice( device ) ); + OperationHelper helper( partitionModelForDevice( device ), this ); if ( partition->roles().has( PartitionRole::Extended ) ) { @@ -420,8 +447,6 @@ PartitionCoreModule::deletePartition( Device* device, Partition* partition ) job->updatePreview(); jobs << Calamares::job_ptr( job ); } - - refresh(); } void @@ -429,12 +454,10 @@ PartitionCoreModule::formatPartition( Device* device, Partition* partition ) { auto deviceInfo = infoForDevice( device ); Q_ASSERT( deviceInfo ); - PartitionModel::ResetHelper helper( partitionModelForDevice( device ) ); + OperationHelper helper( partitionModelForDevice( device ), this ); FormatPartitionJob* job = new FormatPartitionJob( device, partition ); deviceInfo->jobs << Calamares::job_ptr( job ); - - refresh(); } void @@ -445,13 +468,11 @@ PartitionCoreModule::resizePartition( Device* device, { auto deviceInfo = infoForDevice( device ); Q_ASSERT( deviceInfo ); - PartitionModel::ResetHelper helper( partitionModelForDevice( device ) ); + OperationHelper helper( partitionModelForDevice( device ), this ); ResizePartitionJob* job = new ResizePartitionJob( device, partition, first, last ); job->updatePreview(); deviceInfo->jobs << Calamares::job_ptr( job ); - - refresh(); } void @@ -461,13 +482,11 @@ PartitionCoreModule::setPartitionFlags( Device* device, { auto deviceInfo = infoForDevice( device ); Q_ASSERT( deviceInfo ); - PartitionModel::ResetHelper( partitionModelForDevice( device ) ); + OperationHelper( partitionModelForDevice( device ), this ); SetPartFlagsJob* job = new SetPartFlagsJob( device, partition, flags ); deviceInfo->jobs << Calamares::job_ptr( job ); PartitionInfo::setFlags( partition, flags ); - - refresh(); } QList< Calamares::job_ptr > @@ -573,13 +592,11 @@ PartitionCoreModule::refreshPartition( Device* device, Partition* ) // the loss of the current selection. auto model = partitionModelForDevice( device ); Q_ASSERT( model ); - PartitionModel::ResetHelper helper( model ); - - refresh(); + OperationHelper helper( model, this ); } void -PartitionCoreModule::refresh() +PartitionCoreModule::refreshAfterModelChange() { updateHasRootMountPoint(); updateIsDirty(); @@ -796,7 +813,7 @@ PartitionCoreModule::revertAllDevices() ++it; } - refresh(); + refreshAfterModelChange(); } @@ -827,7 +844,7 @@ PartitionCoreModule::revertDevice( Device* dev ) m_bootLoaderModel->init( devices ); - refresh(); + refreshAfterModelChange(); emit deviceReverted( newDev ); } diff --git a/src/modules/partition/core/PartitionCoreModule.h b/src/modules/partition/core/PartitionCoreModule.h index d61311c8a..704fff322 100644 --- a/src/modules/partition/core/PartitionCoreModule.h +++ b/src/modules/partition/core/PartitionCoreModule.h @@ -54,6 +54,25 @@ class PartitionCoreModule : public QObject { Q_OBJECT public: + /** + * This helper class calls refresh() on the module + * on destruction (nothing else). It is used as + * part of the model-consistency objects, along with + * PartitionModel::ResetHelper. + */ + class RefreshHelper + { + public: + RefreshHelper( PartitionCoreModule* module ); + ~RefreshHelper(); + + RefreshHelper( const RefreshHelper& ) = delete; + RefreshHelper& operator=( const RefreshHelper& ) = delete; + + private: + PartitionCoreModule* m_module; + }; + /** * @brief The SummaryInfo struct is a wrapper for PartitionModel instances for * a given Device. @@ -192,7 +211,7 @@ Q_SIGNALS: void deviceReverted( Device* device ); private: - void refresh(); + void refreshAfterModelChange(); /** * Owns the Device, PartitionModel and the jobs