GNOME Bugzilla – Bug 759726
Implement Partition object polymorphism
Last modified: 2016-04-26 15:54:24 UTC
This enhancement is to implement Partition object polymorphism in the GParted code. This is required for my strategy of implementing LUKS support using PartitionLUKS class derived from the Partition class. C++ requires the use of pointers and references to objects, and not copying objects directly to avoid object slicing. Reference: Bug 627701 - option to encrypt new partitions (using LUKS)
Created attachment 317759 [details] [review] Implement Partition object polymorphism (Draft 1) Hi Curtis, Here's patchset draft 1. It's here so you can have a look and raise any issues. It's draft until I do a final review of the changes myself and I implement LUKS read-only support patchset on top of this. Thanks, Mike Chose to use raw pointers to Partition objects because the code already uses raw pointers, including raw pointers to Operation objects to implement polymorphism. Plus want the code to compile on the oldest still supported distrubions. As raw pointers have been used each Partition object is owned by the class or method which contains the pointer and is responsible for allocation and deallocation. Classes owning pointers to Partition objects are: * PartitionVector: Server the same purpose but replaces std::vector<Partition> used previously. Provides a vector of managed Partition objects, internally using pointers. Includes *_adopt() which is used when scanning the disks and populating the vector. Method allocates the Partition object and gets PartitionVector to take ownership. * Operation and derived classes Operation*: Partition objects are allocated in the derived constructors and deallocated in the derived destructors. * Dialog_Base_Partition and derived classes Dialog_Partition_{Copy,New,Resize_Move}: Single Partition object used as the new designed Partition object created by the dialog. Allocated in the derived constructors and deallocated in the derived destructors. * Win_GParted: Single copied_partition Partition object. Allocated and replaced when needed. Deallocated in the destructor. A few methods in Win_GParted and GParted_Core also own a pointer to a Partition objects locally and allocate and deallocate them.
Hi Mike, First let me say, wow! This is a huge patch set comprising over 7500 lines. I can only imagine how long it took to learn about polymorphism and implement it for GParted partitions. I really appreciate your commit comments and reference links. These helped me to follow the logic of the code changes. Rather than testing this patch set by itself, I plan to test the code in conjunction with: Bug 760080 - Implement read-only LUKS support. Following are some minor spelling mistakes that I encountered while reviewing the patch set: P01: Create PartitionVector class (#759726) - Missing word in commit comment: Just creates PartitionVector class and includes it partition.h so that P02: Stop returning vector of partitions from Dialog_Rescue_Data class (#759726) - no change P03: Use PartitionVector class throughout the code (#759726) - no change P04: Include Partition.h header everywhere it's used - no change P05: Use pointers to Partitions in PartitionVector class (#759726) - no change P06: Add adoption methods for adding items into a PartitionVector (#759726) - Typo in commit comment: is easily worked around by following the Virtual Construtor idiom ^^^^^^^^^^ Constructor P07: Consolidate down to a single insert_unallocated() implementation (#759726) - no change P08: Use Partition pointer adoption everywhere when adding items into a PartitionVector (#759726) - Typo in code comment in GParted_Core::set_device_partitions // Only for libparted reported partition types that we care about: NOMAL, ^^^^^ NORMAL P09: Remove copy (constructing) add item methods from PartitionVector (#759726) - no change P10: Protect partition members within Operation classes (#759726) - Typo in commit comment: partition_priginal, and for OperationCopy class only, partition_copied ^^^^^^^^ original P11: Use pointers to Partitions in Operation classes (#759726) - no change P12: Change copied_partition into pointer (#759726) - Typo in commit comment: allocated, copy construted and deleted. Required as part of the ^^^^^^^^^^ constructed - Typo in commit comment: assigned. Make the compiler enforce this with private decelarations and ^^^^^^^^^^^^^ declarations P13: Change methods to use Partition pointers locally (#759726) - no change P14: Stop unnecessarly coping a Partition in Dialog_Rescue_Data (#759726) - Typos in patch summary line Stop unnecessarly coping a Partition in Dialog_Rescue_Data (#759726) ^^^^^^^^^^^^ ^^^^^^ unnecessary copying P15: Use pointer to Partition in Dialog_Base_Partition and derived classes (#759726) - Typo in commit comment: equlivant to how the Partition objects are managed in the Operation and ^^^^^^^^^ equivalent P16: Replace all Partition object copy assignment (#759726) - no change P17: Implement and use virtul Partition copy constructor clone() (#759726) - Typo in patch summary line Implement and use virtul Partition copy constructor clone() (#759726) ^^^^^^ virtual P18: Simplify GParted_Core::get_fs() - Typo in commit comment: get_fs() use to work by (1) returning the supported capabilities of the ^^^ used - Missing word in commit comment: requested file system found the FILESYSTEMS vector; (2) if not found ^ in - Typo (case) in commit comment: FS_UNKNOWN And return that. ^^^ and P19: Initialise all struct FS members - no change P20: Add FILESYSTEM_MAP[FS_UNALLOCATED] entry - no change P21: Stop relying on specific values of PED_PARTITION_* enum - Typo in commit comment: lp_partition->type to 0 for the type pameter and passing it as a bool ^^^^^^^ parameter - Typo in code comment in GParted_Core::set_device_partitions // GParted is only intrested in partitions with these single bits set: ^^^^^^^^^ interested - Typo in code comment in GParted_Core::set_device_partitions // PED_PARTITION_METADATA bits respectively, are ignore and GParted ^^^^^^ ignored - Typo in code comment in GParted_Core::set_device_partitions // creates it own unallocated partitions and accounts for partition ^^ its P22: Remove unnecessary sector_size parameter from Get_New_Partition methods - no change P23: Add virtual qualifier to derived Operation class destructors - no change P24: Remove unused Partition(path) constructor - no change I look forward to the new capabilities that will become possible when we implement this enhancement. Thanks, Curtis
Created attachment 318338 [details] [review] Implement Partition object polymorphism (v1) Hi Curtis, I must be doing something right with the code if all you are finding is spelling mistakes ;-). So now that the patchset for LUKS read-only support has been submitted (bug 760080) this is now officially patchset v1. - Addressed all your spelling corrections, and a few others. - Makes a few small comment changes. - Also made a change to PartitionVector::replace_at() so that it allocates new resources before releasing old resources. Thanks, Mike
Hi Curtis, To give further confidence in the code with regard correct memory resource handling (new/delete) I ran a set of tests under valgrind and compared the results between current git head and this patchset. Summary: I didn't see anything that looks like a new memory leak or corruption. As you'll see interpreting valgrind output is not easy. Memory leak testing ------------------- Compare different builds: B1) GIT HEAD, 0.24.0 +git-26-g2f6325d B2) this patchset, "Implement Partition object polymorphism (v1)" from comment #3 Create each build with: ./autogen.sh CXXFLAGS='-g -O0' && make clean && cd src && make mv gparted gparted-B1 Test setup: Device md127 with GPT and 2 partitions: ext4 and xfs Tests: (Chosen to exercise pointers to Partition objects). T1) Load gparted and close T2) Load gparted, view every device: md126, md127, sda, sdb, sdc, sdd, sde and close T3) Load gparted, view md127, view partition 1 information and close T4) Load gparted, view md127, create new partition, format new to different fs, delete new partition and close T5) Load gparted, view md127, create new partition, resize/move new, switch to view sdb and close T6) Load gparted, view md127, shrink existing partition, apply and close T7) Load gparted, view md127, grow existing partition, apply and close T8) Load gparted, view md127, copy existing partition 2 into unallocated, copy existing partition 1 into partition 2 and close T9) Load gparted, view md127, format existing partition with a new file system, name partition, check partition, label file system, new uuid, switch to view sdb and close Ran like this: # valgrind --track-origins=yes --leak-check=full ./gpartedbin-B1 2>&1 | sed 's/^M//g;s/==[0-9][0-9]*==/=={PID}==/;s/by 0x[0-9A-F][0-9A-F]*:/by {ADDR}/;s/[^ ]*\/gpartedbin[^ ]*/=={GPARTEDBIN}==/' > gparted-B1-T1.log (Replace ^M with actual Ctrl-M character, typed as Ctrl-V, Ctrl-M. Sed gets rid of most but not all the incidental differences. Look at the LEAK SUMMARY, definitely leaked and mentions of GParted:: name space). Using diff can be useful too to compare results from the same tests with the different builds: # diff -u gparted-B[12]-T1.log I didn't see anything that looks like a new memory leak or corruption. As you'll see interpreting valgrind output is not easy. Deliberate failure ------------------ To be sure I made this change to not free the partition objects in the destructor of the name partition operation: --- a/src/OperationNamePartition.cc +++ b/src/OperationNamePartition.cc @@ -34,8 +34,6 @@ OperationNamePartition::OperationNamePartition( con... OperationNamePartition::~OperationNamePartition() { - delete partition_original; - delete partition_new; partition_original = NULL; partition_new = NULL; } Valgrind output reported several leaks originating in the constructor where the partition_original and partition_new pointers are allocated. Source code: 24 OperationNamePartition::OperationNamePartition( const Device &... 25 const Partitio... 26 const Partitio... 27 { 28 type = OPERATION_NAME_PARTITION; 29 30 this->device = device.get_copy_without_partitions(); >> 31 this->partition_original = partition_orig.clone(); >> 32 this->partition_new = partition_new.clone(); 33 } Valgrind fragments: =={PID}== 433 (280 direct, 153 indirect) bytes in 1 blocks are definitely lost in loss record 6,714 of 7,408 =={PID}== at 0x4A075FC: operator new(unsigned long) (vg_replace_malloc.c:298) =={PID}== by {ADDR} GParted::Partition::clone() const (Partition.cc:34) >>{PID}>> by {ADDR} GParted::OperationNamePartition::OperationNamePartition(GParted::Device const&, GParted::Partition const&, GParted::Partition const&) (OperationNamePartition.cc:31) =={PID}== by {ADDR} GParted::Win_GParted::activate_name_partition() (Win_GParted.cc:2697) =={PID}== 317 (280 direct, 37 indirect) bytes in 1 blocks are definitely lost in loss record 6,672 of 7,408 =={PID}== at 0x4A075FC: operator new(unsigned long) (vg_replace_malloc.c:298) =={PID}== by {ADDR} GParted::Partition::clone() const (Partition.cc:34) >>{PID}>> by {ADDR} GParted::OperationNamePartition::OperationNamePartition(GParted::Device const&, GParted::Partition const&, GParted::Partition const&) (OperationNamePartition.cc:32) =={PID}== by {ADDR} GParted::Win_GParted::activate_name_partition() (Win_GParted.cc:2697) Thanks, Mike
Hi Mike, Patch set (v1) from comment #3 to implement partition object polymorphism has been committed to the git repository. The relevant git commits can be viewed at the following links: Create PartitionVector class (#759726) https://git.gnome.org/browse/gparted/commit/?id=fa9827344551cf7df265d4a1b8bbc570ac266425 Stop returning vector of partitions from Dialog_Rescue_Data class (#759726) https://git.gnome.org/browse/gparted/commit/?id=81337141d70536a0f7a6b4b244b06f4a47163d87 Use PartitionVector class throughout the code (#759726) https://git.gnome.org/browse/gparted/commit/?id=fae909897e92b55aed0624ec8ccf221806e23ef4 Include Partition.h header everywhere it's used https://git.gnome.org/browse/gparted/commit/?id=48d898ebfdf1b80a7798a63c0fdaceb9a46f1a83 Use pointers to Partitions in PartitionVector class (#759726) https://git.gnome.org/browse/gparted/commit/?id=06b8a3a14a89c5e9e33b7232c8646b93466be79a Add adoption methods for adding items into a PartitionVector (#759726) https://git.gnome.org/browse/gparted/commit/?id=fdbd86f1ead93f0bfa328466de9895e43fc2b131 Consolidate down to a single insert_unallocated() implementation (#759726) https://git.gnome.org/browse/gparted/commit/?id=2a2a99b2bf4a99e30b4892fd1d73228dc15d51f9 Use Partition pointer adoption everywhere when adding items into a PartitionVector (#759726) https://git.gnome.org/browse/gparted/commit/?id=f6b45a0429bf4e52024651625aeb9433f190e8ee Remove copy constructing add item methods from PartitionVector (#759726) https://git.gnome.org/browse/gparted/commit/?id=504a2d8393dbe6f5db4deb88a346b813e265d5a3 Protect partition members within Operation classes (#759726) https://git.gnome.org/browse/gparted/commit/?id=6fd37c074529aaada18ec04baf098133b695b859 Use pointers to Partitions in Operation classes (#759726) https://git.gnome.org/browse/gparted/commit/?id=b516b1093c6ac0e65b87e125204d49a7a7fdf0d3 Change copied_partition into a pointer (#759726) https://git.gnome.org/browse/gparted/commit/?id=ea8ab702f74a2c41d046b065bd28abb02c0fd233 Change methods to use Partition pointers locally (#759726) https://git.gnome.org/browse/gparted/commit/?id=4d8578646c7deed70cbc1c7192128af172c9c4c3 Stop unnecessarily coping a Partition in Dialog_Rescue_Data (#759726) https://git.gnome.org/browse/gparted/commit/?id=9f4e6909c59ac8a767ca8c74f6a62cb9f9b39a7d Use pointer to Partition in Dialog_Base_Partition and derived classes (#759726) https://git.gnome.org/browse/gparted/commit/?id=4a6cbcd0f1e6238bdf8c06b592f19538267c6d23 Replace all Partition object copy assignment (#759726) https://git.gnome.org/browse/gparted/commit/?id=656e1709ffdf9853495e4ac6380896a103ac8ef4 Implement and use virtual Partition copy constructor clone() (#759726) https://git.gnome.org/browse/gparted/commit/?id=320e166c039918ad93b9dcaa811ee0181e2e318d Simplify GParted_Core::get_fs() https://git.gnome.org/browse/gparted/commit/?id=49664f3ca38f3fa0d70304a44bfa795f8d88244e Initialise all struct FS members https://git.gnome.org/browse/gparted/commit/?id=1a4cefb9601de47a1d4fdd31d60be9e37fadda2d Add FILESYSTEM_MAP[FS_UNALLOCATED] entry https://git.gnome.org/browse/gparted/commit/?id=7870a92b8097b3b8067cc87b891451d5164e9b74 Stop relying on specific values of PED_PARTITION_* enum https://git.gnome.org/browse/gparted/commit/?id=3b3d8e44b684041c776659f49198ae9d2e1acc6a Remove unnecessary sector_size parameter from Get_New_Partition methods https://git.gnome.org/browse/gparted/commit/?id=24fa553385005a0beb97c8d0b1899b3d4a5df227 Add virtual qualifier to derived Operation class destructors https://git.gnome.org/browse/gparted/commit/?id=561e2203d5bdb98a4f5e6b1d2a33da51f56710e1 Remove unused Partition(path) constructor https://git.gnome.org/browse/gparted/commit/?id=cd64d6503b5c669b21ec53d24baccfe20efe9916 Curtis
This enhancement was included in the GParted 0.26.0 release on April 26, 2016.