Make especially the Volume Identifier length limit code simpler to
understand and therefore easier to maintain.
Bug 784533 - Add support for UDF file system
UDF label is stored in the Logical Volume Identifier which has space for
either 126 Latin1 or 63 UCS-2 characters. For compatibility reasons
with older versions of blkid, the possibly truncated UDF label is also
stored in the Volume Identifier which only has space for 30 Latin1 or 15
UCS-2 characters.
Because versions of mkudffs prior to 1.1 damage the label if it contains
non-ASCII characters, make sure GParted does not call such versions of
mkudffs with a non-ASCII character label.
Bug 784533 - Add support for UDF file system
Add support for detecting UDF file systems and formatting hard disks
with revision 2.01 UDF file systems using udftools. Formatting optical
disks or any other media types is not supported yet. Changing label or
UUID after formatting is not supported as the tools do not yet exist.
Bug 784533 - Add support for UDF file system
For large output a lot of time is used copying capturebuf to callerbuf
to provide a Glib::ustring copy of the buffer for the update callback.
However update callbacks are only used when commands are run to apply
operations by FileSystem::execute_command() and their output is
incrementally displayed in the UI. Whereas update callbacks are never
used when commands are used to query information via
Utils::execute_command().
Stop performing interim copying of capturebuf to callerbuf when there
are no update callbacks registered as it is unnecessary.
Time to read portions of the recorded fsck.fat output via
fat16::set_used_sectors() and intermediate copies aren't required:
1 MiB 10 MiB 122 MiB
old code : 0.074 sec 1.41 sec 210 sec [3:30]
new code : 0.063 sec 0.56 sec 6.57 sec
Bug 777973 - Segmentation fault on bad disk
If PipeCapture reads a NUL byte in the middle of what is expected to be
a multi-byte UTF-8 character then PipeCapture either returns the
captured characters to the previous update or loops forever depending on
whether the end of the stream is encountered before the read buffer is
full or not. This is equivalent to saying whether the NUL byte occurs
within the last 512 bytes of the output or not.
This is caused by a bug in g_utf8_get_char_validated() reporting that a
partial UTF-8 character has been found when the NUL byte is encountered
in the middle of a multi-byte character even though more bytes are
available in the length specified buffer. g_utf8_get_char_validated()
is always stopping at the NUL byte assuming it is working with a NUL
terminated string.
Workaround this by checking for g_utf8_get_char_validated() claiming a
partial UTF-8 character has been found when in fact there are at least
enough bytes in the read buffer to instead determine that it is really
an invalid UTF-8 character.
Reference:
Bug 780095 - g_utf8_get_char_validated() stopping at nul byte even
for length specified buffers
https://bugzilla.gnome.org/show_bug.cgi?id=780095
Bug 777973 - Segmentation fault on bad disk
A user had a very corrupted FAT file system such that fsck.fat produced
122 MiB of output. GParted has to read this output to get the file
system usage information. However GParted takes more than 48 hours to
read the 122 MiB of output, while using 100% CPU time and is
unresponsive for the duration.
Modified fsck.fat to output just the first 1 MiB of output and used perf
to capture performance data from GParted reading that output:
# perf -g -F 1999 -- ./gpartedbin
# perf report --stdio
67.84% Glib::ustring::replace [4.23s]
17.67% g_utf8_pointer_to_offset [1.10s]
8.48% g_utf8_offset_to_pointer [0.53s]
[ 6.01% (everything else) ] [0.38s]
[100.00% TOTAL ] [6.24s]
And to read the first 10 MiB of output the performance figures are:
92.95% Glib::ustring::replace [257.44s]
4.35% g_utf8_pointer_to_offset [ 12.05s]
2.13% g_utf8_offset_to_pointer [ 5.90s]
[ 0.58% (everything else) ] [ 1.61s]
[100.00% TOTAL ] [277.00s]
See how the total time is increasing non-linearly, 44 times longer for
only 10 times as much data. This is because of the exponential increase
in time spent in Glib::ustring::replace.
After a lot of experimentation I came to the conclusion that
Glib::ustrings are not appropriate for storing and editing large buffers
of data, sizes megabytes and above. The issues are that iterators are
invalid after the content changes and replacing UTF-8 characters by
index gets exponentially slower as the size of the string increases.
Hence the > 48 hours of 100% CPU time to read and apply the line
discipline to the 122 MiB of fsck.fat output. See code comment for a
more detailed description of the issues found.
Rewrote OnReadable() to use Glib::ustrings as little as possible.
Instead using buffers and vectors of fixed width data types allowing for
fast access using pointers and indexes (converted to pointers by the
compiler with simple arithmetic). Again see code comment for a more
detailed description of the implementation.
Repeating the performance capture with the new code for the first 1 MiB
of fsck.fat output:
63.34% memcpy [0.35s]
[ 36.66% (everything else) ] [0.21s]
[100.00% TOTAL ] [0.56s]
And for the first 10 MiB of fsck.fat output:
96.66% memcpy [63.60s]
[ 3.34% (everything else) ] [ 2.20s]
[100.00% TOTAL ] [65.80s]
Simple timings taken to read portions of the fsck.fat output (when not
using perf):
1 MiB 10 MiB 122 MiB
old code : 6.2 sec 277 sec > 48 hours
(4:37)
new code : 0.6 sec 66 sec 17262 sec
(1:06) (4:47:42)
Performance of the code is still non-linear because of the assignment
of the ever growing capturebuf to callerbuf for every block of input
read. This is required to generate a consistent Glib::ustring copy of
the input for the update callback. However this is much faster than
before, and I have a plan for further improvements.
Bug 777973 - Segmentation fault on bad disk
As the source code is managed in GIT and there is a .gitignore file in
the top level directory specifying file names to exclude from version
control, then the old per-directory .cvsignore files for CVS are
redundant.
Add the only missing and applicable entry from src/.cvsignore of '.libs'
to .gitignore and remove all the .cvsignore files.
Using the default MiB alignment, creating an MSDOS logical partition
between two other existing logical partitions fails with this error
dialog:
(-) <b>An error occurred while applying the operations</b>
See the details for more information.
<b>IMPORTANT</b>
If you want support, you need to provide the saved details!
See http://gparted.org/save-details.htm for more information.
[ OK ]
and these operation details:
+ libparted messages
- Unable to satisfy all constraints on the partition.
This bug was introduced by this commit included in GParted 0.23.0:
90e3ed68fc
Shallow copy Device object into Operation object (#750168)
The commit message claimed that the deep copied Partition objects inside
the Device inside the Operation object are never accessed. This turned
out not to be true. Win_GParted::Add_Operation() uses them as part of
snap_to_alignment() which updates requested partition boundaries to
account for alignment requirements and the space needed for EBR
(Extended Boot Record) preceding logical partitions.
In this case the new logical partition was trying to be created over the
top of the EBR for the following logical partition because
snap_to_alignment() wasn't aware of its existence.
Fix by making Add_Operation() and snap_to_alignment() refer to the
current device, as displayed in the UI, rather than the shallow copy
included in the Operation object. Hopefully now it is true that the
not copied vector of Partition objects in the Device object in each
Operation object are never accessed.
Bug 779339 - enforce at least 1 MiB "free space following"
The Operation class already provided find_index_extended() method and
was used in the Operation and derived classes where required. It
returns the index to the extended partition in the PartitionVector
object, or -1 when no extended partition exists.
There were several cases of the same functionality being open coded in
GParted_Core and Win_GParted. Therefore move the implementation to
find_extended_partition() in PartitionVector compilation unit and use
this implementation everywhere.
The clear_mountpoints parameter has never been used since
add_mountpoint*() were first added [1][2]. clear_mountpoints() method
[3] is available to provide this functionality and used. Therefore
removed unused parameter and code.
[1] add_mountpoints() added 2006-03-15
9532c3cad1
Made Partition::mountpoints private
[2] add_mountpoint() added 2011-12-16
208083f11d84dbd4f186271a3cdbf5170db259f8b8
Display LVM2 VGNAME as the PV's mount point (#160787)
[3] clear_mountpoint() added 2006-03-19
ad9f2126e7
fixed issues with copying (see also #335004) cleanups + added FIXME added
Now that resizing of encrypted file systems is implemented add growing
of the open LUKS mapping as part of the check repair operation.
Resizing an encrypted file system requires the LUKS mapping to be open
to access the file system within; therefore it also requires libparted
and kernel support for online partition resizing. This limits resizing
to the latest distributions with libparted >= 3.2 and kernel >= 3.6.
However growing an open LUKS mapping as part of a check repair operation
doesn't require resizing the partition. Therefore route via offline
grow of LUKS to avoid those extra, unnecessary requirement. This does
mean that offline LUKS grow artificially requires cryptsetup, but that is
not really significant as even opening LUKS requires cryptsetup.
So now checking an encrypted file system on even the oldest
distributions does:
1) runs FSCK on the encrypted file system;
2) grows the encryption volume to fill the partition;
3) grows the file system to fill the encryption mapping.
Bug 774818 - Implement LUKS read-write actions NOT requiring a
passphrase
Moving of closed LUKS is simply enabled by luks .move capability being
set and requires no further coding.
Resizing of encrypted file systems requires both the LUKS mapping and
encrypted file system within to be resized in the right order for both
shrinking and growing. To keep the code simple split resizing of plain
and encrypted into separate functions.
Bug 774818 - Implement LUKS read-write actions NOT requiring a
passphrase
Changing the Resize/Move dialog code to also handle PartitionLUKS
objects was considered too complicated. Instead create an unencrypted
equivalent using clone_as_plain(), pass that to the Resize/Move dialog
and finally apply the change back using Partition*::resize().
Bug 774818 - Implement LUKS read-write actions NOT requiring a
passphrase
Add a resize() method to both Partition and PartitionLUKS classes. They
take a reference Partition object, and update the position, size and
file system usage of *this Partition to match. This is ready for taking
a partition returned from Resize/Move dialog and applying the change.
Bug 774818 - Implement LUKS read-write actions NOT requiring a
passphrase
Implement a specialist PartitionLUKS clone method. Creates a new
Partition object which has the same space usage as the source encrypted
file system, but is a plain file system. Namely, the overhead of the
LUKS header has been added to the file system usage. This is ready for
feeding this representation of the partition to the Resize/Move dialog.
Bug 774818 - Implement LUKS read-write actions NOT requiring a
passphrase
Already have:
Utils::get_filesystem_string(FS_EXT2) -> "ext2"
virtual Partition::get_filesystem_string() -> "ext2"
virtual PartitionLUKS::get_filesystem_string() -> "[Encrypted] ext2"
Add these:
Utils::get_encrypted_string() -> "[Encrypted]"
Utils::get_filesystem_string(false, FS_EXT2) -> "ext2"
Utils::get_filesystem_string(true, FS_EXT2) -> "[Encrypted] ext2"
This is ready for use of Utils::get_filesystem_string(true, FS_EXT2)
when composing the preview of a format of an encrypted file system by
Win_GParted::activate_format().
Bug 774818 - Implement LUKS read-write actions NOT requiring a
passphrase
When composing, describing and implementing the operation just need the
code to query and set the Partition object directly containing the file
system, instead of the enclosing encryption mapping to make it work.
The operation details for setting a new UUID on an encrypted ext4 file
system become:
Set a new random UUID on [Encrypted] ext4 file system on /dev/sdb4
+ calibrate /dev/sdb4
+ Set UUID on /dev/mapper/sdb4_crypt to a new, random value
+ tune2fs -U random /dev/mapper/sdb4_crypt
tune2fs 1.41.12 (17-May-2010)
Also note the now documented rule in apply_operation_to_disk() which
says each operation must leave the status of the encryption mapping and
file system as it found it.
Bug 774818 - Implement LUKS read-write actions NOT requiring a
passphrase
Provide and use a single interface for getting the file system string
for display, regardless of whether the partition is encrypted or the
encryption mapping is active or not.
Example return values for get_filesystem_string() for different types
and states of Partition objects:
1) Plain ext4 file system: -> "ext4"
2) Closed encrypted: -> "[Encrypted]"
3) Open encrypted ext4 file system: -> "[Encrypted] ext4"
This simplifies the code in TreeView_Detail::create_row() which sets the
file system type displayed in the main window. The same method will
then also be used when setting the operation description as each
operation is updated to handle encrypted file systems.
Bug 774818 - Implement LUKS read-write actions NOT requiring a
passphrase
The previous commit changed how the code behind the main window
retrieved the file system label for display. This is the relevant
changes in TreeView_Detail::create_row():
+ const Partition & filesystem_ptn = partition.get_filesystem_partition();
...
- Glib::ustring temp_filesystem_label = partition.get_filesystem_label();
+ Glib::ustring temp_filesystem_label = filesystem_ptn.get_filesystem_label();
treerow[treeview_detail_columns.label] = temp_filesystem_label;
In the case of an encrypted file system get_filesystem_label() is now
called on the Partition object directly rather than on the outer
Partition object containing the LUKS encryption.
The code behind the Information dialog always obtained and used the
Partition object directly containing the file system to call
get_filesystem_label() since read-only LUKS support was added.
Therefore the virtualised PartitionLUKS::get_filesystem_label() is no
longer needed, so remove it.
Bug 774818 - Implement LUKS read-write actions NOT requiring a
passphrase
There are multiple cases of code wanting to work with the Partition
object directly containing the file system, regardless of whether it is
within a PartitionLUKS object or not. The code had to do something
similar to this to access it:
const Partition * filesystem_ptn = &partition;
if ( partition.filesystem == FS_LUKS && partition.busy )
filesystem_ptn = &dynamic_cast<const PartitionLUKS *>( &partition )->get_encrypted();
...
// Access Partition object directly containing the file system
filesystem_ptn-> ...
Implement and use virtual accessor get_filesystem_partition() which
allows the code to be simplified like this:
const Partition & filesystem_ptn = partition.get_filesystem_partition();
...
// Access Partition object directly containing the file system
filesystem_ptn. ...
Bug 774818 - Implement LUKS read-write actions NOT requiring a
passphrase
More recent versions of blkid don't report an ISO9660 file system on the
whole disk device if partitions can be reports for embedded partitions.
However when querying the whole disk device directly then the expected
ISO9660 file system is reported. For example on CentOS 7 with the
previous ISO images:
# wget http://git.kernel.org/cgit/utils/util-linux/util-linux.git/plain/tests/ts/isosize/sample.iso.gz
# dd if=/dev/zero bs=1M of=/dev/sdc
# zcat sample.iso.gz | dd of=/dev/sdc
# blkid -v
blkid from util-linux 2.23.2 (libblkid 2.23.0, 25-Apr-2013)
# blkid | fgrep /dev/sdc
/dev/sdc1: UUID="2013-01-04-22-05-45-00" LABEL="ARCH_201301" TYPE="iso9660" PTTYPE="dos"
# blkid /dev/sdc
/dev/sdc: UUID="2013-01-04-22-05-45-00" LABEL="ARCH_201301" TYPE="iso9660" PTTYPE="dos"
# wget http://cdimage.debian.org/debian-cd/8.6.0/amd64/iso-cd/debian-8.6.0-amd64-netinst.iso
# dd if=/dev/zero bs=1M of=/dev/sdc
# dd if=debian-8.6.0-amd64-netinst.iso bs=1M of=/dev/sdc
# blkid | fgrep /dev/sdc
/dev/sdc1: UUID="2016-09-17-14-23-48-00" LABEL="Debian 8.6.0 amd64 1" TYPE="iso9660" PTTYPE="dos"
/dev/sdc2: SEC_TYPE="msdos" UUID="17F3-1162" TYPE="vfat"
# blkid /dev/sdc
/dev/sdc: UUID="2016-09-17-14-23-48-00" LABEL="Debian 8.6.0 amd64 1" TYPE="iso9660" PTTYPE="dos"
This behavioural difference with blkid is probably as a result of newer
versions of udev informing the kernel of the partitions embedded within
the ISO9660 image, and not directly as a result of a change in blkid
itself. Older distributions don't have partition entries for the above
ISO images, but CentOS 7 (with udev 219) and later distributions do have
partition entries:
# fgrep sdc /proc/partitions
8 16 8388608 sdc
8 17 252928 sdc1
8 18 416 sdc2
Fix by ensuring that the blkid FS_Info cache has entries for whole disk
devices, even if each entry has all empty attributes because there is a
partition table and not a recognised file system.
Calling blkid on whole disk devices containing partition tables produces
output like this, with newer versions of blkid:
# blkid /dev/sda
/dev/sda: PTTYPE="dos"
# blkid /dev/sdb
/dev/sdb: PTTYPE="gpt"
This will be loaded into the FS_Info cache as a blank entry for the
device by run_blkid_load_cache(). There will be a path name but all the
other attributes will be blank because there are no TYPE, SEC_TYPE, UUID
or LABEL name value pairs. With older versions of blkid no output is
produced at all. In that case load_fs_info_cache_extra_for_path() will
create the same blank entry with just the path name defined.
Bug 771244 - gparted does not recognize the iso9660 file system in
cloned Ubuntu USB boot drives
Move code from GParted_Core::set_devices_thread() performing top level
population of each Device object during the scan of the drives into new
set_device_from_disk() method.
Bug 771244 - gparted does not recognize the iso9660 file system in
cloned Ubuntu USB boot drives
Requires blkid.
Note that FS_LUKS was also moved to more closely match the order in
include/Utils.h
Bug 771244 - gparted does not recognize the iso9660 file system in
cloned Ubuntu USB boot drives
FileSystem::resize() (and derived) is only ever called from a single
location and always with the fill_partition argument supplied [1].
Therefore providing a default for the argument is unnecessary. So
remove it.
[1] Since these two commits from 2006-06-17, before GParted 0.3.0, which
added resize_filesystem() and maximize_filesystem() methods to wrap
calls to p_filesystem->resize() into a single location:
08245cd08c
cleanups in the core and the fs'es (resize)
2d7fb5700b
more cleanups in the core and the fs'es (these changelogs are getting
It made the code look a little messy, is easily resolved in the build
system and made the dependencies more complicated than needed. Each
GParted header was tracked via multiple different names (different
numbers of "../include/" prefixes). For example just looking at how
DialogFeatures.o depends on Utils.h:
$ cd src
$ make DialogFeatures.o
$ egrep ' [^ ]*Utils.h' .deps/DialogFeatures.Po
../include/DialogFeatures.h ../include/../include/Utils.h \
../include/../include/../include/../include/../include/../include/Utils.h \
../include/../include/../include/Utils.h \
After removing "../include/" from the GParted header #includes, just
need to add "-I../include" to the compile command via the AM_CPPFLAGS in
src/Makefile.am. Now the dependencies on GParted header files are
tracked under a single name (with a single "../include/" prefix). Now
DialogFeatures.o only depends on a single name to Utils.h:
$ make DialogFeatures.o
$ egrep ' [^ ]*Utils.h' .deps/DialogFeatures.Po
../include/DialogFeatures.h ../include/Utils.h ../include/i18n.h \
Linux-swap is recreated as part of copy, resize and move operations and
the code was special cased to implement that by calling the linux-swap
specific resize method. However the displayed text always said "growing
file system" and then proceeded to recreate linux swap. Example
operation:
Copy /dev/sdb1 to /dev/sdb2
...
+ copy file system from /dev/sdb1 to /dev/sdb2
Partition copy action skipped because linux-swap file system does not contain data
+ grow file system to fill the partition
+ create new linux-swap file system
+ mkswap -L"" -U "77d939ef-54d6-427a-a2bf-a053da7eed4c" /dev/sdb2
Setting up swapspace version 1, size = 262140 KiB
LABEL=, UUID=77d939ef-54d6-427a-a2bf-a053da7eed4c
Fix by writing recreate_linux_swap_filesystem() method with better
messaging and use everywhere maximise_filesystem() was previously used
to recreate linux-swap. Also as this is a create step, erase the
partition first to prevent the possibility of any other file system
signatures being found afterwards. Now the operation steps are more
reflective of what is actually being performed.
Copy /dev/sdb1 to /dev/sdb2
...
+ copy file system from /dev/sdb1 to /dev/sdb2
Partition copy action skipped because linux-swap file system does not contain data
+ clear old file system signatures in /dev/sdb2
+ create new linux-swap file system
+ mkswap -L"" -U "77d939ef-54d6-427a-a2bf-a053da7eed4c" /dev/sdb2
Setting up swapspace version 1, size = 262140 KiB
LABEL=, UUID=77d939ef-54d6-427a-a2bf-a053da7eed4c
Bug 775932 - Refactor mostly applying of operations
resize_filesystem() was meeting two different needs:
1) when called with fill_partition = false it generated operation
details;
2) when called from maximize_filesystem() with fill_partition = true it
skipped generating any operation details;
then ran the switch statement to select the resize implementation. So
extract the common switch statement into new method
resize_filesystem_implement().
Then observe that the only time resize_filesystem() was called to grow
the file system was when re-creating linux-swap. Therefore change that
call to use maximize_filesystem() and rename to shrink_filesystem() and
modify the operation detail messages to match.
Bug 775932 - Refactor mostly applying of operations
Make the methods called below apply_operation_to_disk() follow a
standard naming convention:
* Contains "_partition"
Uses libparted to query or change the partition in the disk label
(partition table).
E.g.:
calibrate_partition()
create_partition()
delete_partition()
name_partition()
resize_move_partition()
set_partition_type()
* Contains "_filesystem"
Manipulates the file system within the partition, mostly using the
FileSystem and derived class methods.
E.g.:
create_filesystem()
remove_filesystem()
label_filesystem()
copy_filesystem()
erase_filesystem_signatures()
check_repair_filesystem()
resize_filesystem()
maximize_filesystem()
* Other
Compound method calling multiple partition and file system related
apply methods.
E.g.:
create()
format()
copy()
resize_move()
resize()
move()
Rename:
Delete() -> delete_partition()
change_uuid() -> change_filesystem_uuid()
Bug 775932 - Refactor mostly applying of operations
Until now an encryption mapping has been modelled as a Partition object
similar to a partition like this:
.encrypted.device_path = "/dev/sdb1"
.encrypted.path = "/dev/mapper/sdb1_crypt"
.encrypted.whole_device = false
.encrypted.sector_start = // start of the mapping in the partition
.encrypted.sector_end = // end of the mapping in the partition
However accessing device_path in the start to end sector range is not
equivalent to accessing the partition path as it doesn't provide access
to the encrypted data. Therefore existing functions which read and
write partition data (GParted file system copying and signature erasure)
via libparted using the device_path won't work and will in fact destroy
the encrypted data. This could be coded around with an extra case in
the device opening code, however it is not necessary.
An encrypted block special device /dev/mapper/CRYPTNAME looks just like
a whole disk device because it doesn't contain a partition and the file
system it contains starts at sector 0 and goes to the end. Therefore
model an encryption mapping in the same way a whole disk device is
modelled as a Partition object like this:
.encrypted.device_path = "/dev/mapper/sdb1_crypt"
.encrypted.path = "/dev/mapper/sdb1_crypt"
.encrypted.whole_device = true
.encrypted.sector_start = 0
.encrypted.sector_end = // size of the encryption mapping - 1
Now GParted file system copy and erasure will just work without any
change. Just need to additionally store the LUKS header size, which was
previous stored in the sector_start, for use in the
get_sectors_{used,unused,unallocated}() calculations.
Bug 775932 - Refactor mostly applying of operations
Split out the switch statement selecting the copy implementation and
associated copy file system operation detail message into a separate
copy_filesystem() method, matching how a number of other operations are
coded. This is why the previous copy_filesystem() methods needed
renaming.
Re-write the remaining copy() into if-operation-fails-return-false style
to simplify it. Re-write final complicated conditional check repair and
maximise file system into separate positive if conditions for swap and
larger partition to make it understandable.
The min_size parameter to copy() was queried from the partition_src
parameter also passed to copy(). Drop the parameter and query inside
copy() instead.
Bug 775932 - Refactor mostly applying of operations
Rename GParted_Core methods:
copy_filesystem(4 params) -> copy_filesystem_internal()
copy_filesystem(5 params) -> copy_filesystem_internal()
copy_filesystem(10 params) -> copy_blocks()
See the following commit for the desire to do this.
Bug 775932 - Refactor mostly applying of operations
Files were named Block_Copy and the class was named block_copy. Change
to the primary naming convention of CamelCase class name and matching
file names.
Also make CopyBlocks::copy_block() a private method as it is only used
internally within the class.
Bug 775932 - Refactor mostly applying of operations
These two methods had a lot of repeated and common code. Both determine
if the partition has any pending operations, notify the user that
changing the busy status can not be performed, and report any errors
when changing the status.
Extract the common code into sub-functions check_toggle_busy_allowed()
and show_toggle_failure_dialog() to handle showing the message dialogs.
Also refactor toggle_busy_state() to make it clear that it handles 5
cases of swapon, swapoff, activate VG, deactivate VG and unmount file
system.
Also remove out of date comment near the top of toggle_busy_state()
stating there can only be pending operations for inactive partitions is
out of date. Some file systems can be resized while online and
partition naming is allowed whatever the busy status of the file system.
Bug 775932 - Refactor mostly applying of operations
The primary reason to refactor unmount_partition() is to pass the
Partition object to be unmounted, rather than use member variable
selected_partition_ptr so that it doesn't have to handle the differences
between encrypted and non-encrypted Partition objects. The calling
function can deal with that instead. Then there were lots of small
reasons to change almost every other line too:
* Return success or failure rather than updating a passed pointer with
the result. Leftover from when the function used to be a separate
thread:
commit 52a2a9b00a
Reduce threading (#685740)
* Pass updated error string by reference rather than by pointer. Likely
another leftover.
* Stop borrowing the updated error string as a local variable for the
error output from the umount command. Use new umount_error local
variable instead. Was bad practice making the code harder to
understand.
* Rename failed_mountpoints to skipped_mountpoints to better reflect
that it contains the mount points not attempted to be unmounted
because two or more file systems are mounted at that point.
* Rename errors to umount_errors to better reflect it contains the
errors from each failed umount command.
* Document the reason for mount points being skipped.
* Update the skipped mount points message to state definitely why they
could not be unmounted rather than stating most likely.
* Simplify logic processing the error cases and return value.
* Made static because it no longer accesses any class members.
* Remove out dated "//threads.." comment from the header. Another
leftover from when the function use to be a separate thread.
Bug 775932 - Refactor mostly applying of operations
Reorder the parameters into the same order in which they occur in the
row, i.e. Name first, then Mount Point and finally Label. Rename local
variables in load_partitions(1 param) and parameters of
load_partitions(5 params) prefixing with "show_" to make it clearer the
variables track if that column will be displayed or not.
create_row() populates the values for each row to be displayed in the UI
from the relevant Partition object. However load_partitions(5 params)
independently decided if the Name, Mount Point and Label columns were
empty and should be displayed.
Getting the mount point value is more complex for encrypted file systems
because it has to call get_mountpoints() on the inner encrypted
Partition object. load_partitions(5 params) didn't account for this.
Fix by making create_row() both copy the values into each row and at the
same time check if they are empty to decide if they should be displayed
or not.
Bug 775475 - Mount Point column displayed for encrypted file systems
even when empty
If the GParted main window is closed before the initial device load
completed gpartedbin never exits. The main window closes but the
process sits there idle forever. Subsequently running GParted reports
this error:
# gparted
The process gpartedbin is already running.
Only one gpartedbin process is permitted.
If the user is running GParted from a desktop menu they will never see
this error so they will never know why GParted won't start any more.
More technically, it is if the main window is closed before the
Win_GParted::on_show() callback completes.
I assume the Gtk main loop doesn't setup the normal quit handling until
the on_show() callback finishes drawing the main window for the first
time. Following this hint [1], move the initial device load from the
on_show() callback to immediately after it completes by using a run once
idle callback setup in on_show().
This looks exactly the same to the user except now gpartedbin exits when
the main window is closed during the initial device load. Note that
GParted finished the device load before exiting. This is exactly the
same as happens when exiting during subsequent device refreshes.
[1] How to know when a Gtk Window is fully shown?
http://stackoverflow.com/questions/14663212/how-to-know-when-a-gtk-window-is-fully-shown
"If you want to know when your main window appears the first time,
it is far easier (and saner) add a g_idle_add after your show_all
call."
Bug 771816 - GParted never exits if the main window is closed before the
initial device load completes
This commit stopped setting the text colours in the Partition, File
System and Mount Point columns to avoid hard coding text colours making
them impossible to read when using GNOME's High Contrast Inverse theme:
ff2a6c00dd
Changes post gparted-0.3.6 - code recreation from Source Forge
* src/TreeView_Detail.cc: Removed text_color hard coding
- Removed hard coding of Partition and Filesystem text_color
which was based on if partition was TYPE_UNALLOCATED.
- Removed hard coding of Mount text_color which was based
on if partition was busy. Lock symbol provides this indicator.
- Closes GParted bug #413810 - Don't hardcode text colour in
partition list
Now remove the remaining vestiges left behind. Remove the unused color
text and mount_text_color columns from the tree model. Also remove
setting of the column attributes which set the colour of the text in the
tree view from those unused columns in the tree model.
Unnecessary history. Added by:
b179990dc9
show greyed-out mountpoint of unmounted partitions in the treeview
as an improved way to identify partitions
Bug #333027 - Displaying unmounted partitions' default mount points
in grey
and by commit only in CVS history:
Bart Hakvoort <...> 2004-08-22 15:06:45
Made text in Partition column darkgrey for unallocated. this offers
more visual difference between partitions and unallocated space
The GParted_Core::mount_info and GParted_Core::fstab_info maps and the
methods that manipulate them are self-contained. Therefore move them to
a separate Mount_Info module and reduce the size of the monster
GParted_Core slightly.
The FS_Info module has a pseudo multi-object interface and used the
constructor to load the cache. However all the data in the class is
static. An FS_Info object doesn't contain any member variables, yet was
needed just to call the member functions.
Make all the member functions static removing the need to use any
FS_Info objects and provide an explicit load_cache() method.
The Proc_Partitions_Info has a pseudo multi-object interface and uses
the constructor to load the cache. However all the data in the class is
static. A Proc_Partitions_Info object doesn't contain any member
variables, yet was needed just to call the member functions.
Make all the member functions static removing the need to use any
Proc_Partitions_Info objects and provide and explicit load_cache()
method.
Vol_id has been retired and removed from all supported distributions.
See earlier commit "Remove use of retired vol_id from FS_Info module
(#767842)" for more details. Therefore remove it's use from GParted
entirely.
GParted is already reading /proc/partitions to get whole disk device
names. The file also contains the major, minor device number of every
partition. Use this information to pre-populate the cache in the
BlockSpecial class.
# cat /proc/partitions
major minor #blocks name
8 0 20971520 sda
8 1 512000 sda1
8 2 20458496 sda2
...
9 3 1047552 md3
259 2 262144 md3p1
259 3 262144 md3p2
...
253 0 18317312 dm-0
253 1 2097152 dm-1
253 2 8383872 dm-2
253 3 1048576 dm-3
Note that for Device-Mapper names (dm-*) the kernel is not using the
canonical user space names (mapper/*). There is no harm in
pre-populating the cache with these names and will help if tools report
them too. It is just that for DMRaid, LVM and LUKS, GParted uses the
canonical /dev/mapper/* names so will still have to call stat() once for
each such name.
For plain disks (sd*) and Linux Software RAID arrays (md*) the kernel
name is the common user space name too, therefore matches what GParted
uses and pre-populating does avoid calling stat() at all for these
names.
Bug 767842 - File system usage missing when tools report alternate block
device names
Creation of every BlockSpecial object used to result in a stat() OS
call. On one of my test VMs debugging with 4 disks and a few partitions
on each, GParted refresh generated 541 calls to stat() in the
BlockSpecial(name) constructor. However there were only 45 unique
names. So on average each name was stat()ed approximately 12 times.
Cache the major, minor number associated with each name after starting
with a cleared cache for each GParted refresh. This reduces these
direct calls to stat() to just the 45 unique names.
Bug 767842 - File system usage missing when tools report alternate block
device names
The FS_Info cache is loaded from "blkid" output and compares block
special names. Therefore switch to using BlockSpecial objects so that
comparisons are performed by the major, minor device number instead.
Bug 767842 - File system usage missing when tools report alternate block
device names
FS_Info module caches the output from blkid as a single string and uses
regular expressions to find the line matching the requested block
special file name. This is not compatible with using BlockSpecial
objects to represent block special files, and perform matching by major,
minor device number. Therefore parse the blkid output into a vector of
structures containing the needed fields, ready for switching to
BlockSpecial objects in the following patch.
Interface to the module remains unchanged.
Bug 767842 - File system usage missing when tools report alternate block
device names
Vol_id was removed from udev 142, released 2009-05-13, and udev switched
to using blkid instead [1]. All currently supported distributions use
later versions of udev (or systemd after the udev merge), except for
RedHat / CentOS 5 with udev 095. However RedHat / CentOS 5 does provide
blkid and vol_id is found in udev specific /lib/udev directory not on
the PATH. Therefore effectively vol_id is not available on any
supported distribution and blkid is always available. Therefore remove
use of vol_id from the FS_Info module. Less code to refactor and test
in following changes.
[1] delete vol_id and require util-linux-ng's blkid
http://git.kernel.org/cgit/linux/hotplug/udev.git/commit/?id=f07996885dab45102492d7f16e7e2997e264c725
Bug 767842 - File system usage missing when tools report alternate block
device names
The SWRaid_Info cache is loaded from "mdadm" command output and
/proc/mdstat file. It contains the member name which is used to access
the cache, therefore switch to using BlockSpecial objects so that
comparison is performed using the major, minor device number.
Bug 767842 - File system usage missing when tools report alternate block
device names
The LUKS_Info module cache is loaded from "dmsetup" command and compares
block special files, therefore switch to using BlockSpecial objects so
that comparisons are performed by major, minor device number.
Bug 767842 - File system usage missing when tools report alternate block
device names
Small optimisation which avoids constructing an extra BlockSpecial
object when determining if a btrfs member is mounted. Rather than
extracting the name from the BlockSpecial object in
btrfs::get_mount_device() and re-constructing another BlockSpecial
object from that name in GParted_Core::is_dev_mounted(), pass the
BlockSpecial object directly.
Bug 767842 - File system usage missing when tools report alternate block
device names
There are no known errors which affect the remaining caches in GParted.
However those caches which compare block special devices will be changed
to use BlockSpecial objects so comparison is by major, minor device
number rather than by name.
Change btrfs member cache loaded from "btrfs filesystem show" output to
use BlockSpecial objects.
Bug 767842 - File system usage missing when tools report alternate block
device names
On some distributions having btrfs on top of LUKS encrypted partitions,
adding a second device and removing the first device used to mount the
file system causes GParted to no longer be able to report the file
system as busy or the mount points themselves.
For example, on CentOS 7, create a single btrfs file system and mount
it. The provided /dev/mapper/sdb1_crypt name is reported, via
/proc/mounts, as the mounting device:
# cryptsetup luksFormat --force-password /dev/sdb1
# cryptsetup luksOpen /dev/sdb1 sdb1_crypt
# mkfs.btrfs -L encrypted-btrfs /dev/mapper/sdb1_crypt
# mount /dev/mapper/sdb1_crypt /mnt/1
# ls -l /dev/mapper
total 0
lrwxrwxrwx. 1 root root 7 Jul 2 14:15 centos-root -> ../dm-1
lrwxrwxrwx. 1 root root 7 Jul 2 14:15 centos-swap -> ../dm-0
crw-------. 1 root root 10, 236 Jul 2 14:15 control
lrwxrwxrwx. 1 root root 7 Jul 2 15:14 sdb1_crypt -> ../dm-2
# fgrep btrfs /proc/mounts
/dev/mapper/sdb1_crypt /mnt/1 btrfs rw,seclabel,relatime,space_cache 0 0
Add a second device to the btrfs file system:
# cryptsetup luksFormat --force-password /dev/sdb2
# cryptsetup luksOpen /dev/sdb2 sdb2_crypt
# btrfs device add /dev/mapper/sdb2_crypt /mnt/1
# ls -l /dev/mapper
...
lrwxrwxrwx. 1 root root 7 Jul 2 15:12 sdb2_crypt -> ../dm-3
# btrfs filesystem show /dev/mapper/sdb1_crypt
Label: 'encrypted-btrfs' uuid: 45d7b1ef-820c-4ef8-8abd-c70d928afb49
Total devices 2 FS bytes used 32.00KiB
devid 1 size 1022.00MiB used 12.00MiB path /dev/mapper/sdb1_crypt
devid 2 size 1022.00MiB used 0.00B path /dev/mapper/sdb2_crypt
Remove the first mounting device from the btrfs file system. Now the
non-canonical name /dev/dm-3 is reported, via /proc/mounts, as the
mounting device:
# btrfs device delete /dev/mapper/sdb1_crypt /mnt/1
# btrfs filesystem show /dev/mapper/sdb2_crypt
Label: 'encrypted-btrfs' uuid: 45d7b1ef-820c-4ef8-8abd-c70d928afb49
Total devices 1 FS bytes used 96.00KiB
devid 2 size 1022.00MiB used 144.00MiB path /dev/mapper/sdb2_crypt
# fgrep btrfs /proc/mounts
/dev/dm-3 /mnt/1 btrfs rw,seclabel,relatime,space_cache 0 0
# ls -l /dev/dm-3
brw-rw----. 1 root disk 253, 3 Jul 2 15:12 /dev/dm-3
GParted loads the mount_info mapping from /proc/mounts and with it the
/dev/dm-3 name. When GParted is determining if the encrypted btrfs file
system is mounted or getting the mount points it is using the
/dev/mapper/sdb2_crypt name. Therefore no information is found and the
file system is incorrectly reported as unmounted.
Fix by changing mount_info and fstab_info to use BlockSpecial objects
instead of strings so that matching is performed by major, minor device
numbers rather than by string compare. Note that as BlockSpecial
objects are used as the key of std::map [1] mappings operator<() [2]
needs to be provided to order the key values.
[1] std::map
http://www.cplusplus.com/reference/map/map/
[2] std::map::key_comp
http://www.cplusplus.com/reference/map/map/key_comp/
Bug 767842 - File system usage missing when tools report alternate block
device names
In some cases creating an LVM2 Physical Volume on top of a DMRaid array
reports no usage information and this partition warning:
Unable to read the contents of this file system!
Because of this some operations may be unavailable.
The cause might be a missing software package.
The following list of software packages is required for lvm2
pv file system support: lvm2.
For example on Ubuntu 14.04 LTS (with GParted built with
--enable-libparted-dmraid) create an LVM2 PV in a DMRaid array
partition. GParted uses this command:
# lvm pvcreate -M 2 /dev/mapper/isw_bacdehijbd_MyArray0p2
But LVM reports the PV having a different name:
# lvm pvs
PV VG Fmt Attr PSize PFree
/dev/disk/by-id/dm-name-isw_bacdehijbd_MyArray0p2 lvm2 a-- 1.00g 1.00g
This alternate name is loaded into the LVM2_PV_Info module cache. Hence
when GParted queries partition /dev/mapper/isw_bacdehijbd_MyArray0p2 it
has no PV information against that name and reports unknown usage.
However they are actually the same block special device; major 252,
minor 2:
# ls -l /dev/mapper/isw_bacdehijbd_MyArray0p2
brw-rw---- 1 root disk 252, 2 Jul 2 11:09 /dev/mapper/isw_bacdehijbd_MyArray0p2
# ls -l /dev/disk/by-id/dm-name-isw_bacdehijbd_MyArray0p2
lrwxrwxrwx 1 root root 10 Jul 2 11:09 /dev/disk/by-id/dm-name-isw_bacdehijbd_MyArray0p2 -> ../../dm-2
# ls -l /dev/dm-2
brw-rw---- 1 root disk 252, 2 Jul 2 11:09 /dev/dm-2
To determine if two names refer to the same block special device their
major, minor numbers need to be compared, instead of string comparing
their names.
Implement class BlockSpecial which encapsulates the name and major,
minor numbers for a block special device. Also performs comparison as
needed. See bug 767842 comments 4 and 5 for further investigation and
decision for choosing to implement a class.
Replace name strings in the LVM2_PV_Info module with BlockSpecial
objects performing correct block special device comparison.
Bug 767842 - File system usage missing when tools report alternate block
device names
Now Device and Partition objects only have a single path,
get_alternate_paths() is never called. Remove the method and population
of the private alternate_paths_cache member that went with it.
Bug 767842 - File system usage missing when tools report alternate block
device names
To reflect that there is now only a single path in the Partition object
now. Also get rid of the now unneeded optional clear_paths parameter
which was only relevant when there was a vector of paths.
Bug 767842 - File system usage missing when tools report alternate block
device names
Change from a vector of paths to a single path member in the Partition
object. Remove add_paths() and get_paths() methods. Keep add_path()
and get_path().
Bug 767842 - File system usage missing when tools report alternate block
device names
To reflect that there is only a single path in the Device object now.
Also get rid of the now unneeded optional parameter which was only
relevant when there was a vector of paths.
Bug 767842 - File system usage missing when tools report alternate block
device names
Background
GParted stored a list of paths for Device and Partition objects. It
sorted this list [1][2] and treated the first specially as that is what
get_path() returned and was used almost everywhere; with the file system
specific tools, looked up in various *_Info caches, etc.
[1] Device::add_path(), ::add_paths()
[2] Partition::add_path(), ::add_paths()
Mount point display [3] was the only bit of GParted which really worked
with the path list. Busy file system detection [4] just used the path
provided by libparted, or for LUKS /dev/mapper/* names. It checked that
single path against the mounted file systems found from /proc/mounts,
expanded with additional block device names when symlinks were
encountered.
[3] GParted_Core::set_mountpoints() -> set_mountpoints_helper()
[4] GParted_Core::set_device_partitions() -> is_busy()
GParted_Core::set_device_one_partition() -> is_busy()
GParted_Core::set_luks_partition() -> is_busy()
Having the first path, by sort order, treated specially by being used
everywhere and virtually ignoring the others was wrong, complicated to
remember and difficult code with. As all the additional paths were
virtually unused and made no difference, remove them. The "improved
detection of mountpoins, free space, etc.." benefit from commit [5]
doesn't seem to exist. Therefore simplify to a single path for Device
and Partition objects.
[5] commit 6d8b169e73
changed the way devices and partitions store their device paths.
Instead of holding a 'realpath' and a symbolic path we store paths
in a list. This allows for improved detection of mountpoins, free
space, etc..
This patch
Simplify the Device object from a vector of paths to a single path.
Remove add_paths() and get_paths() methods. Keep add_path() and
get_path() for now.
Bug 767842 - File system usage missing when tools report alternate block
device names
Recognise GRUB2 core.img boot code written to a partition without a file
system. Such setups are possible/likely with GPT partitioned disks as
there is a specific partition type reserved for it [1][2]:
21686148-6449-6E6F-744E-656564454649 (BIOS Boot partition)
[1] GUID Partition Table, Partition types
https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_type_GUIDs
[2] BIOS boot partition
https://en.wikipedia.org/wiki/BIOS_boot_partition
Bug 766989 - zfsonline support - need file system name support for ZFS
type codes
Composing these operations caused GParted to abort on an assert failure:
(1) Check an existing partition,
(2) Create a new partition,
(3) Delete new partition.
# ./gpartedbin
======================
libparted : 2.4
======================
**
ERROR:OperationCheck.cc:40:virtual GParted::Partition& GParted::OperationCheck::get_partition_new(): assertion failed: (false)
Aborted (core dumped)
# gdb ./gpartedbin core.8876 --batch --quiet --ex backtrace -ex quit
[New Thread 8876]
[New Thread 8879]
[Thread debugging using libthread_db enabled]
Core was generated by `./gpartedbin'.
Program terminated with signal 6, Aborted.
#0 0x000000361f2325e5 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
64 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
#0 0x000000361f2325e5 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1 0x000000361f233dc5 in abort () at abort.c:92
#2 0x0000003620a67324 in g_assertion_message (domain=<value optimized out>, file=<value optimized out>, line=<value optimized out>, func=0x50f400 "virtual GParted::Partition& GParted::OperationCheck::get_partition_new()", message=0x1d37a00 "assertion failed: (false)") at gtestutils.c:1358
#3 0x0000003620a678f0 in g_assertion_message_expr (domain=0x0, file=0x50f1a8 "OperationCheck.cc", line=40, func=0x50f400 "virtual GParted::Partition& GParted::OperationCheck::get_partition_new()", expr=<value optimized out>) at gtestutils.c:1369
#4 0x0000000000498e21 in GParted::OperationCheck::get_partition_new (this=0x1d1bb30) at OperationCheck.cc:40
#5 0x00000000004c66ec in GParted::Win_GParted::activate_delete (this=0x7fff031c3e30) at Win_GParted.cc:2068
...
When Win_GParted::activate_delete() was stepping through the operation
list removing operations (2 & 3 in the above recreation steps) which
related to the new partition never to be created it called
get_partition_new() on all operations in the list. This included
calling get_partition_new() on the check operation (1 in the above
recreation steps). As partition_new was not set or used by the check
operation get_partition_new() asserted false and crashed GParted.
Fix by populating the partition_new member in OperationCheck objects,
thus allowing get_partition_new() to be called on the object. As a
check operation doesn't change any partition boundaries or file system
attributes, just duplicate the new partition from the original
partition.
Bug 767233 - GParted core dump on assert failure in
OperationDelete::get_partition_new()
The previous commit (Fix crash reading NTFS usage when there is no
/dev/PTN entry) identified that the FileSystem member variable "index"
is too small on 64-bit machines. Also this member variable stores no
FileSystem class information and was being used as a local variable.
Replace with local variables of the of the correct type, wide enough to
store the npos not found value.
Bug 764658 - GParted crashes when reading NTFS usage when there is no
/dev/PTN entry
As part of the internal block copy operation 5 initial ranges of blocks
are copied using different block sizes to determine the fastest. Then
the remainder is copied using the fastest block size. Each of these
copies reports progress independently, so during the benchmarking phase
the progress bar flashes 5 times as it goes from 0 to 100% in a fraction
of a second, before showing the progress of the remainder.
This looks bad, so report a single progress bar for all the ranges of
blocks copied in a single copy operation.
Already have variables done and length which track progress within each
copied range; and total_done which records amount copied in previous
ranges. Just add total_length to allow overall progress to be reported.
Bug 762367 - Use a single progress bar for the whole of the internal
copy operation
Copying of ntfs is performed using ntfsclone, which writes progress
indication to standard output like this:
# ntfsclone -f /dev/sdb2 /dev/sdb1 2> /dev/null
NTFS volume version: 3.1
Cluster size : 4096 bytes
Current volume size: 21474832384 bytes (21475 MB)
Current device size: 21474836480 bytes (21475 MB)
Scanning volume ...
100.00 percent completed
Accounting clusters ...
Space in use : 1832 MB (8.5%)
Cloning NTFS ...
100.00 percent completed
Syncing ...
Add ntfsclone progress tracker for ntfsclone command. Deliberately
doesn't stop the progress bar. See comment in ntfs::clone_progress()
for the explanation.
Bug 762366 - Add progress bar to NTFS file system specific copy method
The timed progress tracking callback for execution of xfs copy follows
this pattern:
sigc::connection c;
...
c = Glib::signal_timeout().connect( ... sigc::mem_fun( *this, &xfs::copy_progress ) ..., 500 /*ms*/ );
... execute_command( ... );
c.disconnect();
As with output progress tracking callbacks for ext2/3/4 and ntfs file
system specific commands, pass the callback slot and a flag into
execute_command() and connect the timed callback inside. This
simplified the pattern to:
... execute_command( ...|EXEC_PROGRESS_TIMED,
static_cast<TimedSlot>( sigc::mem_fun( *this, &xfs::copy_progress ) ) );
NOTE:
The type of sigc::mem_fun() doesn't allow the compiler to choose between
the two overloaded variants of execute_command() with the fourth
parameter of either (full types without typedefs of StreamSlot and
TimedSlot respectively):
sigc::slot<void, OperationDetail *> stream_progress_slot
sigc::slot<bool, OperationDetail *> timed_progress_slot
Therefore have to cast the result of all callback slots to the relevant
type. Hence:
static_cast<StreamSlot>( sigc::mem_fun( *this, &{CLASS}::{NAME}_progress ) )
static_cast<TimedSlot>( sigc::mem_fun( *this, &xfs::copy_progress ) )
References:
* [sigc] Functor not resolving between overloaded methods with
different slot types
https://mail.gnome.org/archives/libsigc-list/2016-February/msg00000.html
* Bug 306705 - Can't overload methods based on different slot<>
parameters.
https://bugzilla.gnome.org/show_bug.cgi?id=306705
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
For the relevant stream from a file system specific command being
tracked, there were 2 callbacks attached: update_command_output() and
update_command_progress(). When called, update_command_progress() just
emitted signal_progress to call the file system specific progress
tracker callback. Like this:
signal_update.emit() -> update_command_output()
-> update_command_progress()
signal_progress.emit() -> {CLASS}::{NAME}_progress()
Instead just connect the file system specific progress tracker callback
directly to signal_update and bypass the unnecessary
update_command_progress() method and the signal_progress signal. Like
this:
signal_update.emit() -> update_command_output()
-> {CLASS}::{NAME}_progress()
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
All the output progress tracking callbacks for execution of ext2/3/4 and
ntfs file system specific commands followed this pattern:
sigc::connection c = signal_progress.connect( sigc::mem_fun( *this, &ext2::..._progress ) );
bool success = ! execute_command( ... );
c.disconnect();
return success;
Instead, pass the callback slot and a flag into execute_command() and
connect the callback inside. This simplifies the pattern to:
return ! execute_command( ...|EXEC_PROGRESS_STDOUT,
sigc::mem_fun( *this, &ext2::..._progress ) );
Note that as the progress tracking callbacks are only registered against
updates to the relevant stream from the tracked commands they won't be
called when the other stream is updated any more.
Also note that signal_progress is a member of the FileSystem class and
derived objects so lives as long as GParted is running, therefore the
progress tracking callbacks need explicitly disconnecting at the end of
execute_command(). However signal_update is a member of the PipeCapture
class of which the output and error local variables in execute_command()
are types. Therefore there is no need to explicitly disconnect the
signal_update callbacks as they will be destructed along with the
callback slots when they go out of scope at the end of the
execute_command() method.
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
Remove unused members: fraction and progress_text from the
OperationDetail class now that the ProgressBar class has superseded
their use. This also allows removal of timer_global member from the
copy_blocks class. Timer_global was only used to track the elapsed time
copying blocks and allow the remaining time to be estimated and written
into progress_text. The ProgressBar class also does this itself
internally.
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
Most of the file system specific command progress trackers followed this
pattern:
void {CLASS}::{NAME}_progress( OperationDetail *operationdetail )
{
ProgressBar & progressbar = operationdetail->get_progressbar();
// parse output for progress and target values
if ( // have progress and target values )
{
if ( ! progressbar.running() )
progressbar.start( target );
progressbar.update( progress );
operationdetail->signal_update( *operationdetail );
}
else if ( // found progress finished )
{
if ( progressbar.running() )
progressbar.stop();
operationdetail->signal_update( *operationdetail );
}
}
That is a lot of repetition handling progress bar updates and
OperationDetail object update signalling. Remove the need for direct
access to the single ProgressBar object and provide these two
OperationDetail methods instead:
// Start and update in one
run_progressbar( progress, target, optional text_mode );
stop_progressbar();
Now the file system specific command progress trackers can become:
void {CLASS}::{NAME}_progress( OperationDetail *operationdetail )
{
// parse output for progress and target values
if ( // have progress and target values )
{
operationdetail->run_progressbar( progress, target );
}
else if ( // found progress finished )
{
operationdetail->stop_progressbar();
}
}
Make ProgressBar::get_progressbar() a private method to enforce use of
the new way to access the progress bar via the run_progress() and
stop_progressbar() methods. Then make the Dialog_Progress a friend
class to OperationDetail so that the Apply pending operations dialog can
still access the single ProgressBar object for its querying needs.
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
Using e2image to copy a file system looks like this. (Intermediate
progress lines which are constantly overwritten are indicated with ">").
# e2image -ra -p /dev/sdb4 /dev/sdb5
e2image 1.42.13 (17-May-2015)
Scanning inodes...
> Copying 0 / 276510 blocks (0%)
> Copying 8845 / 276510 blocks (3%)
> Copying 48433 / 276510 blocks (18%)
> Copying 77135 / 276510 blocks (28%)
> Copying 111311 / 276510 blocks (40%)
> Copying 137039 / 276510 blocks (50%)
> Copying 166189 / 276510 blocks (60%) 00:00:03 remaining at 108.20 MB/s
> Copying 190285 / 276510 blocks (69%) 00:00:03 remaining at 106.19 MB/s
> Copying 209675 / 276510 blocks (76%) 00:00:02 remaining at 102.38 MB/s
> Copying 238219 / 276510 blocks (86%) 00:00:01 remaining at 103.39 MB/s
> Copying 256692 / 276510 blocks (93%) 00:00:00 remaining at 100.27 MB/s
Copied 276510 / 276510 blocks (100%) in 00:00:10 at 108.01 MB/s
Note that the copying figures are reported in file system block size
units and the progress information is written to stderr, hence needing
these two previous commits:
Record file system block size where known (#760709)
Call any FS specific progress trackers for stderr updates too (#760709)
Add progress tracking function for e2image command. Also tracks when
the text progress indicator has passed in the output so that the
progress bar can be stopped as well as started when needed.
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
Record the file system block size in the Partition object. Only
implemented for file systems when set_used_sectors() method has already
parsed the value or can easily parse the value from the existing
executed command(s).
Needed for ext2/3/4 copies and moves performed using e2image so that
they can be tracked in bytes by the ProgressBar class as e2image reports
progress in file system block size units.
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
XFS uses a file system specific method to copy the partition using
"xfsdump | xfsrestore". Monitor progress by periodically querying the
destination file system usage and comparing to the source file system
usage. Use 0.5 seconds as the polling interval to match that used by
the internal block copying algorithm.
NOTE:
The number of used blocks in the source and destination file system will
be very close but may not match exactly. I have seen an XFS copy finish
with the following progress text:
1.54 GiB of 1.50 GiB copied (-00:00:02 remaining)
Allow the progress bar to overrun like this as it is informing the user
that it actually copied a little more data and took a little longer than
expected. Needs these two previous commits to correctly round and
format the negative time remaining:
Fix rounding of negative numbers (#760709)
Fix formatting of negative time values (#760709)
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
Adapt the ext2 resize progress tracker to the new ProgressBar class.
Also update the progress function to track when text progress bars have
completely passed in the output so that the progress bar can be stopped
as well as started when needed.
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
1) Multiple progress bars
The OperationDetail class contains member fraction which is used to feed
data to the current operation progress bar shown in the Applying pending
operations dialog. Dialog_Progress::on_signal_update() gets called for
every updated OperationDetail object and depending on whether fraction
is > 0.0 or not, switches between showing a growing or pulsing progress
bar. This leads to the conclusion that every OperationDetail object
currently being updated is effectively driving the single on screen
progress bar with different data.
The Copy_Blocks code is careful to update text and faction in a single
OperationDetail object and everything is good. The on screen progress
bar is switched into growing mode and then grows to 100%.
Since external command output is updated in real time [1] there are two
OperationDetail objects, one for stdout and one for stderr, which are
updated whenever data is read from the relevant stream. Also now that
progress is interpreted from some external command output [2][3][4] a
separate OperationDetail object is getting updated with the progress
fraction. (Actually the grandparent OperationDetail of the ones
receiving stdout and stderr updates as used by the file system specific
*_progress() methods). In the normal case of an external command
which is reporting it's progress two OperationDetails are constantly
being updated together, the OperationDetail object tracking stdout and
it's grandparent receiving progress fraction updates. This causes the
the code in Dialog_Progress::on_signal_update() to constantly switch
between growing and pulsing progress bar mode. The only reason this
doesn't flash the progress bar is because the stdout OperationDetail
object is updated first and before the 100 ms timeout fires to pulse the
bar, it's grandparent is updated with the new fraction to keep growing
the bar instead.
2) Common code
The Copy_Blocks code currently tracks the progress of blocks copied
against target amount, which it has to do anyway. That information is
then used to generate the text and fraction to update into the
OperationDetail object and drive the on screen progress bar. This same
level of tracking is wanted for the XFS and ext2/3/4 file system
specific copy methods.
Conclusion and solution
Having multiple sources of progress bar data is a problem and makes it
clear that there must be only one source of progress data. Also some
code can be shared for tracking the amount of blocks copied and
generating the display.
Therefore have a single ProgressBar object which is used everywhere.
This commit
It just creates a single ProgressBar object which is available via all
OperationDetail objects and Copy_Blocks is updated accordingly. Note
that the ProgressBar still contains debugging and that the GUI progress
bar of the current operation is still driven via the fraction member in
any OperationDetail object.
Referenced commits:
[1] 52a2a9b00a
Reduce threading (#685740)
[2] ae434579e1
Display progress for e2fsck (#467925)
[3] baea186138
Display progress for mke2fs (#467925)
[4] 57b028bb8e
Display progress during resize (#467925)
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
Write a generic progress bar class. Has the following features:
* Has separate progress and target numbers, rather than a single
completion fraction, to enable the the next feature.
* Optionally generates text reporting the amount of data copied using
the progress and target numbers like this:
"1.00 MiB of 16.00 MiB copied"
* After running for 5 seconds, also add estimated remaining time.
(Waits to allow the data copying rate to settle down a little before
estimating the remaining time). Looks like this:
"1.00 MiB of 16.00 MiB copied (00:01:59) remaining)"
The ProgressBar class is not driving the visual progress bar yet. It
has just been added into the internal block copy algorithm and generates
debug messages showing the progress bar is operating correctly.
Debugging looks like this:
DEBUG: ProgressBar::start(target=2.0636e+09, text_mode=PROGRESSBAR_TEXT_COPY_BYTES)
DEBUG: ProgressBar::update(progress=1.30023e+08) m_fraction=0.0630081 m_text="124.00 MiB of 1.92 GiB copied"
DEBUG: ProgressBar::update(progress=2.67387e+08) m_fraction=0.129573 m_text="255.00 MiB of 1.92 GiB copied"
DEBUG: ProgressBar::update(progress=4.0475e+08) m_fraction=0.196138 m_text="386.00 MiB of 1.92 GiB copied"
...
DEBUG: ProgressBar::update(progress=1.13351e+09) m_fraction=0.549289 m_text="1.06 GiB of 1.92 GiB copied (00:00:04 remaining)"
DEBUG: ProgressBar::update(progress=1.26249e+09) m_fraction=0.611789 m_text="1.18 GiB of 1.92 GiB copied (00:00:04 remaining)"
DEBUG: ProgressBar::update(progress=1.39041e+09) m_fraction=0.67378 m_text="1.29 GiB of 1.92 GiB copied (00:00:03 remaining)"
...
DEBUG: ProgressBar::update(progress=1.97552e+09) m_fraction=0.957317 m_text="1.84 GiB of 1.92 GiB copied (00:00:00 remaining)"
DEBUG: ProgressBar::update(progress=2.0636e+09) m_fraction=1 m_text="1.92 GiB of 1.92 GiB copied"
DEBUG: ProgressBar::stop()
Bug 760709 - Add progress bars to XFS and EXT2/3/4 file system specific
copy methods
At the moment any messages for an encrypted file system aren't shown,
only messages from the outer PartitionLUKS object are shown. Also in
Win_GParted::activate_paste() the selected Partition object, possibly
a derived PartitionLUKS, is cloned and the messages cleared.
Therefore a set of accessor methods must be provided to query and modify
partition messages. Messages will be stored in the Partition object to
which they are added and retrieved from all. So in the case of a
derived PartitionLUKS they will be retrieved from the messages vector of
the PartitionLUKS object itself and the messages vector for the
encrypted file system it contains.
To replace code like this in GParted_Core:
partition_temp->messages = messages;
We might naturally provide a set_messages() method which assigns the
messages vector and is used like this:
partition_temp->set_messages( messages );
However on a PartitionLUKS object what should set_messages() do? By the
name it will replace any existing messages in the PartitionLUKS object
itself, but what should happen to the messages for the contained
encrypted Partition object? Should they be cleared or left alone?
Rather than implement set_messages() with unclear semantics implement
append_messages(), which in the PartitionLUKS object case will clearly
leave any messages for the contained encrypted Partition object alone.
Append_messages() is then used to add messages as the Partition or
PartitionLUKS objects when populating the data in GParted_Core.
Bug 760080 - Implement read-only LUKS support
For LUKS formatted partitions add an encryption section into the
Information dialog and display the type of encryption, path, UUID and
status of the encryption.
The file system section continues to display appropriate file system
details, including the partition graphic with the file system specific
border colour and correct usage. The details will either be of a plain
file system, an encrypted file system, or nothing when there is no open
dm-crypt mapping, leaving the encrypted file system inaccessible.
Should there be LUKS encryption directly within LUKS encryption then the
details of the inner encryption will be displayed in the file system
section. However this configuration will not be further supported by
GParted.
Bug 760080 - Implement read-only LUKS support
There is already the set of methods in the Partition class to report the
file system usage. Virtualise them and provide PartitionLUKS specific
implementations to calculate the usage of a file system wrapped in LUKS
encryption.
See the ascii art and comment in PartitionLUKS.cc for the details of
those calculations.
Bug 760080 - Implement read-only LUKS support
LUKS headers don't provide any concept of label. Also there is already
the method Partition::get_filesystem_label() for getting the *file
system* label, so virtualise it and provide an appropriate
implementation to get the label of an encrypted file system represented
within a derived PartitionLUKS object. This causes the label to be
displayed correctly in the main window.
It also happens to display the encrypted file system label in the
Information dialog for a LUKS formatted partition. However the whole
Information dialog will be addressed differently in a following commit.
Bug 760080 - Implement read-only LUKS support
In the File System column in the GUI, when there is an open dm-crypt
mapping, display the colour square for the encrypted file system within
and the text as "[Encrypted] FSTYPE". For closed mappings nothing can
be known about the encrypted file system within so continue to display a
purple square and the text "[Encrypted]".
Looks like:
Partition | File System
...
/dev/sdb3 # ext4
v /dev/sdb4 * # extended
/dev/sdb5 # [Encrypted]
/dev/sdb6 * # [Encrypted] unknown
/dev/sdb7 * # [Encrypted] ext4
Bug 760080 - Implement read-only LUKS support
Partition object represents a region of a disk and the file system
within. GParted always displays the colour base of the type of the file
system. Therefore remove the color member and always look it up from
the type of the file system as needed.
This makes one less member that will need virtual accessor methods with
different handling in the derived PartitionLUKS class.
Bug 760080 - Implement read-only LUKS support
When there exists an open dm-crypt mapping, populate the encrypted
Partition object representing the encrypted file system.
Bug 760080 - Implement read-only LUKS support
Absolute minimum implementation of a PartitionLUKS class which can be
constructed, polymorphically copied and destroyed. Contains an
"encrypted" member of type Partition to represent the encrypted file
system within the LUKS format.
Create PartitionLUKS objects instead of base Partition objects when a
LUKS formatted partition is found. Only the base Partition object
member values have been populated, and the "encrypted" member remains
blank at this point.
Bug 760080 - Implement read-only LUKS support
This is the equivalent change as made to set_mountpoints() in an earlier
commit. Change GParted_Core::set_used_sectors() from being called with
a vector of partitions and processing them all to being called per
partition. This is in preparation for calling set_used_sectors() on a
single Partition object inside a PartitionLUKS object.
Bug 760080 - Implement read-only LUKS support
Previously GParted_Core::set_mountpoints() was called with a vector of
partitions and processed them all. Now make set_mountpoints() process a
single partition and push the calls to it down one level from
set_devices_thread() into set_device_partitions() and
set_device_one_partition(). This is in preparation for having an
encrypted file system represented as a Partition object inside a
PartitionLUKS object and needing to call set_mountpoints() for the inner
single Partition object.
Bug 760080 - Implement read-only LUKS support
Only load the LUKS_Info cache of active dm-crypt mappings when the first
LUKS partition is encountered. Not needed from a performance point of
view as the longest that I have ever seen "dmsetup table --target crypt"
take to run is 0.05 seconds. Just means that the dmsetup command is
only run when there are LUKS partitions and the information is needed.
Bug 760080 - Implement read-only LUKS support
Populate the used, unused and unallocated figures in the Partition
object for a LUKS formatted partition. See comment in
luks::set_used_sectors() for the rational of what is used, unused and
unallocated.
As that rational mentions, a LUKS header does not store the size of the
encrypted data and is assumed to extend to the end of the partition by
the tools which start the mapping.
An underlying block device of 128 MiB (131072 KiB).
# sfdisk -s /dev/sde
131072
An active LUKS mapping at offset 2 MiB (4096 512-byte sectors) and
length 126 MiB (129024 KiB, 258048 512-byte sectors).
# sfdisk -s /dev/mapper/sde_crypt
129024
# cryptsetup status sde_crypt
/dev/mapper/sde_crypt is active.
type: LUKS1
cipher: aes-cbc-essiv:sha256
keysize: 256 bits
device: /dev/sde
offset: 4096 sectors
size: 258048 sectors
mode: read/write
No size/length reported when dumping the LUKS header, just (payload)
offset.
# cryptsetup luksDump /dev/sde
LUKS header information for /dev/sde
Version: 1
Cipher name: aes
Cipher mode: cbc-essiv:sha256
Hash spec: sha1
Payload offset: 4096
MK bits: 256
MK digest: 7f fb ba 40 7e ba e4 3b 2f c6 d0 93 7b f7 05 49 7b 72 d4 ad
MK salt: 4a 5b 54 f9 7b 67 af 6e ef 16 31 0a fe d9 7e 5f
c3 66 dc 8a ed e0 07 f4 45 c3 7c 1a 8d 7d ac f4
MK iterations: 37750
UUID: 0a337705-434a-4994-a842-5b4351cb3778
...
Shrink the LUKS mapping to 64 MiB (65536 KiB, 131072 512-byte sectors).
# cryptsetup resize --size 131072 sde_crypt
# sfdisk -s /dev/mapper/sde_crypt
65536
# cryptsetup status sde_crypt
/dev/mapper/sde_crypt is active.
type: LUKS1
cipher: aes-cbc-essiv:sha256
keysize: 256 bits
device: /dev/sde
offset: 4096 sectors
size: 131072 sectors
mode: read/write
Stop and start the LUKS mapping.
# cryptsetup luksClose sde_crypt
# cryptsetup luksOpen /dev/sde sde_crypt
The size of the LUKS mapping is back to 126 MiB (129024 KiB, 258048
512-byte sectors), extending to the end of the partition.
# sfdisk -s /dev/mapper/sde_crypt
129024
# cryptsetup status sde_crypt
/dev/mapper/sde_crypt is active.
type: LUKS1
cipher: aes-cbc-essiv:sha256
keysize: 256 bits
device: /dev/sde
offset: 4096 sectors
size: 258048 sectors
mode: read/write
Bug 760080 - Implement read-only LUKS support
Also load the starting offset and length of the active dm-crypt mapping
into the LUKS_Info module from the dmsetup output. This provides the
location and size of the encrypted data within the underlying block
device.
Note that dmsetup reports in units of 512 bytes sectors [1], the GParted
LUKS_Info module uses bytes and GParted Partition objects work in device
sector size units. However the actual sector size of a dm-crypt mapping
[2] is the same as that of the underlying block device [3].
# modprobe scsi_debug dev_size_mb=128 sector_size=4096
# fgrep scsi_debug /sys/block/*/device/model
/sys/block/sdd/device/model:scsi_debug
# parted /dev/sde print
Error: /dev/sde: unrecognised disk label
Model: Linux scsi_debug (scsi)
Disk /dev/sde: 134MB
[3] Sector size (logical/physical): 4096B/4096B
Partition Table: unknown
# cryptsetup luksFormat /dev/sde
# cryptsetup luksOpen /dev/sde sde_crypt
# parted /dev/mapper/sde_crypt print
Error: /dev/mapper/sde_crypt: unrecognised disk label
Model: Linux device-mapper (crypt) (dm)
Disk /dev/mapper/sde_crypt: 132MB
[2] Sector size (logical/physical): 4096B/4096B
Partition Table: unknown
# cryptsetup status sde_crypt
/dev/mapper/sde_crypt is active.
type: LUKS1
cipher: aes-cbc-essiv:sha256
keysize: 256 bits
device: /dev/sde
offset: 4096 sectors
size: 258048 sectors
mode: read/write
# dmsetup table --target crypt
...
sde_crypt: 0 258048 crypt aes-cbc-essiv:sha256 0000000000000000000000000000000000000000000000000000000000000000 0 8:64 4096
[1] Both cryptsetup and dmsetup report the offset as 4096 and the size/
length as 258048. 128 MiB / (4096+258048) = 512 byte units, even on a
4096 byte sector size device.
Update debugging of LUKS to this:
# ./gpartedbin
======================
libparted : 2.4
======================
DEBUG: /dev/sdb5: LUKS closed
DEBUG: /dev/sdb6: LUKS open mapping /dev/mapper/sdb6_crypt, offset=2097152, length=534773760
/dev/sde: unrecognised disk label
DEBUG: /dev/sde: LUKS open mapping /dev/mapper/sde_crypt, offset=2097152, length=132120576
Bug 760080 - Implement read-only LUKS support
Provide a minimal implementation of a luks file system class which only
does busy detection.
NOTE:
For now, read-only LUKS support, a LUKS partition will be busy when a
dm-crypt mapping exists. Later when read-write LUKS support is added
GParted will need to look at the busy status of the encrypted file
system within the open LUKS partition and map LUKS partition busy status
to encryption being open or closed.
Bug 760080 - Implement read-only LUKS support
Load basic details of active Device-mapper encryption mappings from the
kernel. Use dmsetup active targets.
# cryptsetup luksFormat /dev/sdb5
# cryptsetup luksFormat /dev/sdb6
# cryptsetup luksOpen /dev/sdb6 sdb6_crypt
# ls -l /dev/mapper/sdb6_crypt /dev/dm-0
lrwxrwxrwx. 1 root root 7 Nov 15 09:03 /dev/mapper/sdb6_crypt -> ../dm-0
brw-rw----. 1 root disk 253, 0 Nov 15 09:03 /dev/dm-0
# ls -l /dev/sdb6
brw-rw----. 1 root disk 8, 22 Nov 15 09:02 /dev/sdb6
# dmsetup table --target crypt
sdb6_crypt: 0 1044480 crypt aes-cbc-essiv:sha256 0000000000000000000000000000000000000000000000000000000000000000 0 8:22 4096
So far just load the mapping name and underlying block device reference
(path or major, minor pair).
Note that all supported kernels appear to report the underlying block
device as major, minor pair in the dmsetup output. Underlying block
device paths are added to the cache when found during a search to avoid
stat(2) call on subsequent searches for the same path.
Prints debugging to show results, like this:
# ./gpartedbin
======================
libparted : 2.4
======================
DEBUG: /dev/sdb5: LUKS closed
DEBUG: /dev/sdb6: LUKS open mapping /dev/mapper/sdb6_crypt
Bug 760080 - Implement read-only LUKS support
Renamed from DEV_MAP_PATH to DEV_MAPPER_PATH. Moved so that the
constant is logically intended for use outside of the DMRaid class.
Also specifically make the string constant have external linkage, rather
than the default internal (static) linkage for constants, so that there
is only one copy of the variable in the program, rather than one copy in
each compilation unit which included DMRaid.h. Namely DMRaid.cc and
GParted_Core.cc.
References:
[1] Proper way to do const std::string in a header file?
http://stackoverflow.com/questions/10201880/proper-way-to-do-const-stdstring-in-a-header-file
[2] What is external linkage and internal linkage in C++
http://stackoverflow.com/questions/1358400/what-is-external-linkage-and-internal-linkage-in-c/1358796#1358796
Bug 760080 - Implement read-only LUKS support
History:
1) The constructor was added by commit:
6d8b169e73
2006-03-14 21:37:47
changed the way devices and partitions store their devicepaths. Instead of
2) Removed from most of the file system specific ::Copy() methods by
commit:
ad9f2126e7
2006-03-19 15:30:20
fixed issues with copying (see also #335004) cleanups + added FIXME added
3) Removed from GParted_Core::copy() method by commit:
7bb7e8a84f
2006-05-23 22:17:34
Use ped_device_read and ped_device_write instead of 'dd' to copy
4) Finally removed from the last place in xfs::Copy() method by commit:
e414b71b73
2012-01-11 19:49:13
Update xfs resize and copy to use new helper functions
The Partition(path) constructor is no longer used. Remove.
When a base class destructor is virtual, derived class destructors are
also virtual [1] even if they don't have the virtual qualifier.
As the Operation destructor is virtual, derived Operation* classes
destructors are virtual too. Add virtual qualifier just to reflect what
the C++ language mandates the compiler implement.
[1] Derived class with non-virtual destructor
http://stackoverflow.com/questions/7403883/derived-class-with-non-virtual-destructor
The sector_size parameter is unnecessary as the value can be retrieved
from the sector size of the selected Partition object on which the
create new, copy & paste or resize/move operation is being performed.
For the create new and resize/move operations it is trivial as the
existing unallocated or in use Partition object on which the operation
is being perform already contains the correct sector size. For the copy
& paste operation, which can copy across disk devices of different
sector sizes, we merely have to use the sector size of the existing
selected (destination) Partition object rather than copied (source)
Partition object. Hence these relevant lines in the new code:
Dialog_Partition_Copy::set_data(selected_partition, copied_partition)
new_partition = copied_partition.clone();
...
new_partition->sector_size = selected_partition.sector_size;
The struct FS constructor initialised every member *except* filesystem
and busy. Then in *most* cases after declaring struct FS, assignments
followed like this:
FS fs;
fs.filesystem = FS_BTRFS;
fs.busy = FS::GPARTED;
But member busy wasn't always initialised.
Add initialisation of members filesystem and busy to the struct FS
constructor. Specify optional parameter to the constructor to set the
filesystem member, or when left off filesystem is initialised to
FS_UNKNOWN.
Final step for full polymorphic handling of Partition objects is to
implement a virtual copy constructor. C++ doesn't directly support
virtual copy constructors, so instead use the virtual copy constructor
idiom [1]. (Just a virtual method called clone() which is implemented
in every polymorphic class and creates a clone of the current object and
returns a pointer to it).
Then replace all calls to the (monomorphic) Partition object copy
constructor throughout the code, except in the clone() implementation
itself, with calls to the new virtual clone() method "virtual copy
constructor".
Also have to make the Partition destructor virtual too [2][3] so that
the derived class destructor is called when deleting using a base class
pointer. C++ supports this directly.
[1] Wikibooks: More C++ Idioms / Virtual Constructor
https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Virtual_Constructor
[2] When to use virtual destructors?
http://stackoverflow.com/questions/461203/when-to-use-virtual-destructors
[3] Virtuality
Guideline #4: A base class destructor should be either public and
virtual, or protected and nonvirtual
http://www.gotw.ca/publications/mill18.htm
Bug 759726 - Implement Partition object polymorphism
SQUASH: When first using pointers to Partition and calling delete
Copy assignment of Partition objects is now only performed in a few
places in the Operation and OperationResizeMove classes when updating
the displayed PartitionVector. (From Refresh_Visual() when each
operation is visually applied to the display_partitions vector; the
new_partition from the operation is copy assigned over the top of the
relevant existing partition in the display_partitions vector).
In general polymorphic copy assignment is complicated [1], and is now
unnecessary given the above limited use. All that is needed is a way to
polymorphically replace one Partition object with another in a
PartitionVector.
First, prevent further use of Partition object copy assignment by
providing a private declaration and no implementation, so the compiler
enforces this. Second implement and use PartitionVector method
replace_at() which replaces a pointer to one Partition object with
another at the specified index in the PartitionVector.
[1] The Assignment Operator Revisited
[Section:] Virtual assignment
http://icu-project.org/docs/papers/cpp_report/the_assignment_operator_revisited.html
Bug 759726 - Implement Partition object polymorphism
Mike Fleetwood