After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 759726 - Implement Partition object polymorphism
Implement Partition object polymorphism
Status: RESOLVED FIXED
Product: gparted
Classification: Other
Component: application
GIT HEAD
Other Linux
: Normal enhancement
: ---
Assigned To: Mike Fleetwood
gparted maintainers alias
Depends on:
Blocks:
 
 
Reported: 2015-12-21 10:41 UTC by Mike Fleetwood
Modified: 2016-04-26 15:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Implement Partition object polymorphism (Draft 1) (288.44 KB, patch)
2015-12-21 21:23 UTC, Mike Fleetwood
none Details | Review
Implement Partition object polymorphism (v1) (289.99 KB, patch)
2016-01-06 15:10 UTC, Mike Fleetwood
none Details | Review

Description Mike Fleetwood 2015-12-21 10:41:48 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)
Comment 1 Mike Fleetwood 2015-12-21 21:23:19 UTC
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.
Comment 2 Curtis Gedak 2016-01-05 21:50:06 UTC
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
Comment 3 Mike Fleetwood 2016-01-06 15:10:16 UTC
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
Comment 4 Mike Fleetwood 2016-01-06 22:24:20 UTC
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
Comment 5 Curtis Gedak 2016-01-30 17:49:52 UTC
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
Comment 6 Curtis Gedak 2016-04-26 15:54:24 UTC
This enhancement was included in the GParted 0.26.0 release on April 26, 2016.