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 \
Simplify conditions checking whether activate/deactivate of an LVM2 PV
is possible. Excluding extended partition type was unnecessary as it
only matters that the file system type is LVM2 PV or not.
Also remove activate/deactivate from the comment above as that check
only determines if the busy state of file systems and swap space can be
toggled.
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
Resizing a file system only checked the .grow and .shrink support
capabilities of the file system, even when perform online resizing.
This wasn't a issue because .online_grow and .online_shrink were always
set from the offline .grow and .shrink capabilities respectively, but
only when online resizing was possible.
However the Device Mapper encryption mapping used for LUKS can only be
resized when online, and not when offline. Therefore the correct
.online_grow and .online_shrink capabilities needs to be checked to
prevent a LUKS resize step failing as not implemented.
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
Attempting to copy or move linux-swap partition was possible when the
mkswap command was unavailable. However as linux-swap is re-created as
part of these operations the mkswap command is a mandatory requirement.
Example copy operation results:
Copy /dev/sdb1 to /dev/sdb2
+ calibrate /dev/sdb1
+ check file system on /dev/sdb1 for errors and (if possible) fix them
+ create empty partition
+ set partition type on /dev/sdb2
+ copy file system from /dev/sdb1 to /dev/sdb2 [SUCCESS]
Partition copy action skipped because linux-swap file system does not contain data
+ grow file system to fill the partition [WARNING]
growing is not available for this file system
Because mkswap was not available the grow action didn't re-create
the linux-swap so the newly copied partition is left without any content
and therefore reported as an unknown partition.
Fix by making copying and moving linux-swap depend on mkswap being
available.
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
The initial check in each of GParted_Core::resize() and move() check for
impossible conditions. (See resize_move() where both functions are
called for confirmation). The old messages could have suggested the
user somehow composed the operation incorrectly if they had ever been
seen.
Make it clear these failures are programming bugs, thus expecting the
user to raise a bug report should they ever be seen. While for now the
checks are obviously impossible they document preconditions for the
functions. More precondition checks are expected to be added later.
Bug 775932 - Refactor mostly applying of operations
Simplify the move() function. Change it into an if-operation-fails-
return-false style and re-write the final overly complicated conditional
check repair and maximise file system.
The final condition said if the file system was linux-swap or the new
partition was larger, perform a check repair and maximize file system.
However this is a move only step with a check at the top of the move()
function ensuring the Partition objects are the same size. So that
simplifies to only checking for linux-swap. As the context is a move
and linux-swap is recreated, performing a check repair first is
unnecessary, so remove that too.
Bug 775932 - Refactor mostly applying of operations
Make the logic at the resize_move_partition() call sites a little
simpler by not having to avoid calling it for whole disk device
partitions. Make it a successful non-operation in that case.
Bug 775932 - Refactor mostly applying of operations
Make the logic at the check_repair_filesystem() call sites a little
easier by not having to avoid calling it for online file systems. The
function handles that itself as a silent non-operation.
Bug 775932 - Refactor mostly applying of operations
Make the logic at the set_partition_type() call sites a little simpler
by not having to avoid calling it for whole disk device Partition
objects. Make set_partition_type() handle those itself as a silent
non-operation.
This is similar to how calibrate_partition() could be called on an
UNALLOCATED Partition object, and update_bootsector() is called on
non-NTFS file system Partition objects. Both are successful and silent
non-operations.
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
Running Device > Attempt Data Rescue... hangs the GParted UI for as long
as gpart takes to scan the whole disk device looking for file system
signatures.
Originally when gpart support was added by commit [1] a separate thread
was created to run gpart. Then most threading was removed by commit [2]
which left gpart running in the main thread blocking the UI.
[1] ef37bdb7de
Added support to lost data recovery using gpart
[2] 52a2a9b00a
Reduce threading (#685740)
guess_partition_table() hand codes using Glib to run the gpart command
asynchronously reading standard output, but it just doesn't run the Gtk
main loop to process events, hence the UI hangs. Instead just use
Utils::execute_command() which handles everything already. It runs the
commands asynchronously, reading output and if being run in the main
thread also calls the Gtk main loop to keep the UI responsive.
Bug 772123 - GParted is unresponsive while gpart is running
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
Again this is to handle a long list of mount points for a single
partition. To prevent the file system status value of "Mounted on [list
of 20 mount points]" causing the dialog to become wider than the screen,
line wrap the text. This instead makes the dialog taller, which already
automatically scrolls vertically as needed.
Bug 771693 - Mount Point column is wider than the screen on openSUSE
using btrfs root with lots of mounted subvolumes
openSUSE 42.1 with default btrfs root installation is heavily using
subvolumes. (Think of btrfs as a storage pool and subvolumes as
individual file systems within the pool for a rough approximation).
Thus the root partition is mounted on many mount points:
# df -k
Filesystem 1K-blocks Used Available Use% Mounted on
...
/dev/sda2 19445760 5820080 13157104 31% /
/dev/sda2 19445760 5820080 13157104 31% /var/lib/libvirt/images
/dev/sda2 19445760 5820080 13157104 31% /var/lib/mysql
/dev/sda2 19445760 5820080 13157104 31% /.snapshots
/dev/sda2 19445760 5820080 13157104 31% /home
/dev/sda2 19445760 5820080 13157104 31% /var/opt
/dev/sda2 19445760 5820080 13157104 31% /usr/local
/dev/sda2 19445760 5820080 13157104 31% /var/tmp
/dev/sda2 19445760 5820080 13157104 31% /var/lib/named
/dev/sda2 19445760 5820080 13157104 31% /var/lib/mariadb
/dev/sda2 19445760 5820080 13157104 31% /var/spool
/dev/sda2 19445760 5820080 13157104 31% /var/lib/pgsql
/dev/sda2 19445760 5820080 13157104 31% /var/lib/mailman
/dev/sda2 19445760 5820080 13157104 31% /srv
/dev/sda2 19445760 5820080 13157104 31% /opt
/dev/sda2 19445760 5820080 13157104 31% /var/crash
/dev/sda2 19445760 5820080 13157104 31% /tmp
/dev/sda2 19445760 5820080 13157104 31% /boot/grub2/i386-pc
/dev/sda2 19445760 5820080 13157104 31% /var/log
/dev/sda2 19445760 5820080 13157104 31% /boot/grub2/x86_64-efi
As the Mount Point column contains all 20 mount points it makes the
column wider than the screen, requiring lots of horizontal scrolling to
see the following columns. (Truncated to just 7 mounts in this
example here).
Partition | File System | Mount Point
/dev/sda1 * | # swap |
/dev/sda2 * | # btrfs | /, /.snapshots, /boot/grub2/i386-pc, /boot/grub2/x86_64-efi, /home, /opt, /srv
Fix by making the Mount Point column resizable and display truncated
text with ellipsis. The column now takes the initial and minimum width
from the width of the "Mount Point" column header text.
Partition | File System | Mount Point | Size
/dev/sda1 * | # swap | | 1.45 GiB
/dev/sda2 * | # btrfs | /, /.snapsho... | 18.54 GiB
Bug 771693 - Mount Point column is wider than the screen on openSUSE
using btrfs root with lots of mounted subvolumes
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
Trying to create a new partition table on a device with active
partitions reports the number of active partitions in the error dialog.
However when there is a busy logical partition the number of reported
busy partitions will be one less than the number of partitions in the
main UI showing the busy symbol.
GParted considers extended partitions as busy when any of the logical
partitions it contains as busy. Display in the main UI reflects this.
Fix Win_GParted::active_partitions_on_device_count() to not exclude
extended partitions from the count.
Found that in some cases usage of active encrypted swap was not working,
but only for the first encrypted swap partition. This only failed on
the first Device Mapper device, dm-0:
# ls -l /dev/mapper/ /dev/dm-*
brw-rw---- 1 root disk 254, 0 Oct 4 20:58 /dev/dm-0
brw-rw---- 1 root disk 254, 1 Oct 4 20:58 /dev/dm-1
/dev/mapper/:
total 0
crw------- 1 root root 10,236 Oct 4 19:48 control
lrwxrwxrwx 1 root root 7 Oct 4 20:58 sdb1_crypt -> ../dm-0
lrwxrwxrwx 1 root root 7 Oct 4 20:58 sdb2_crypt -> ../dm-1
# cat /proc/swaps
Filename Type Size Used Priority
/dev/sda1 partition 1524732 92356 -1
/dev/dm-0 partition 1046524 0 -2
/dev/dm-1 partition 1046524 0 -3
Was failing because the minor number of dm-0 was 0, causing BlockSpecial
operator==() to fall back to name comparison rather than major, minor
number, and GParted name /dev/mapper/sdb1_crypt doesn't match /dev/dm-0.
Found on openSUSE and Ubuntu which don't use LVM by default and don't
already have dm-0 used as an LVM Logical Volume which GParted doesn't
support.
The LINUX ALLOCATED DEVICES document [1] says block special device 0, 0
(major, minor) is not used "reserved as null device number". (Not to
be confused with 1, 3 /dev/null the Null device). All other
non-negative pairs are valid block special device numbers. Therefore
update BlockSpecial operator==() accordingly; compare by major, minor
number when either is greater than 0 falling back to string compare
otherwise. This still fits in with the BlockSpecial() constructor using
major, minor numbers 0, 0 to represent plain files.
[1] LINUX ALLOCATED DEVICES
https://www.kernel.org/doc/Documentation/devices.txt
Bug 771670 - Usage of active encrypted swap is not shown
GParted does not show the usage of active encrypted swap partitions,
instead showing partition warning "Unable to read the contents of this
file system! ...". OS setup:
# ls -l /dev/mapper/sdb4_crypt /dev/dm-3
brw-rw----. 1 root disk 253, 3 Sep 14 07:26 /dev/dm-3
lrwxrwxrwx. 1 root root 7 Sep 14 07:26 /dev/mapper/sdb4_crypt -> ../dm-3
# mkswap -L encrypted_swap /dev/mapper/sdb4_crypt
# swapon /dev/mapper/sdb4_crypt
# cat /proc/swaps
Filename Type Size Used Priority
/dev/sda2 partition 2097148 237632 -1
/dev/dm-3 partition 1046524 0 -2
This is because the code was performing a string compare between the
canonical /dev/mapper/sdb4_crypt name GParted is using and the /dev/dm-3
name reported by the kernel via /proc/swaps. Fix by creating
BlockSpecial objects from the names and compare those so that comparison
is done correctly using major, minor numbers.
Bug 771670 - Usage of active encrypted swap is not shown
The comment became completely unnecessary with the transfer of
mount_info and fstab_info into separate Mount_Info module by commit:
63ec73dfda
Split mount_info and fstab_info maps into separate Mount_Info module
It was never necessary to clear one of the mappings at the end of the
device refresh because it was reloaded at the start of the next device
refresh anyway and it is only a small amount of memory.
Have an unmounted file system within an open encrypted mapping and an
entry in /etc/fstab for the file system like this:
# lsblk
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
...
sdb 8:16 0 8G 0 disk
+-sdb1 8:17 0 1G 0 part
+-sdb1_crypt 253:0 0 1022M 0 crypt
# blkid | grep sdb1
/dev/sdb1: TYPE="crypto_LUKS" ...
/dev/mapper/sdb1_crypt: TYPE="ext4" ...
# ls -l /dev/mapper/sdb1_crypt /dev/dm-0
brw-rw----. 1 root disk 253, 0 Sep 12 19:09 /dev/dm-0
lrwxrwxrwx. 1 root root 7 Sep 12 19:09 /dev/mapper/sdb1_crypt -> ../dm-0
# grep sdb1 /etc/fstab
/dev/mapper/sdb1_crypt /mnt/1 ext4 defaults 0 0
The mount point will be shown twice for the partition:
/mnt/1, /mnt/1
This is because add_node_and_mountpoint() adds two entries for both the
symbolic and real block special names:
map["/dev/mapper/sdb1_crypt"] = ["/mnt/1"]
map["/dev/dm-0"] = ["/mnt/1"]
This was needed for the old code which used string compare to match
block devices so that the mount point could be looked up by either name.
However since bug 767842 introduced major, minor number comparison it
became unnecessary. As both names refer to the same device the mount
point gets added twice to the same entry. Hence display of the double
mount.
map[BlockSpecial{"/dev/mapper/sdb1_crypt", 253, 0}] =
["/mnt/1", "/mnt/1"]
It is always going to be the case that the symbolic link and real block
special names have the same major, minor numbers. That was the
requirement of the BlockSpecial class and the reason for using stat() to
lookup the numbers. Therefore adding entries for both names will always
add duplicate entries. Fix by stop using realpath() to lookup the real
name and adding the duplicate entry.
Introduced by:
a800ca8b68
Add BlockSpecial into mount_info and fstab_info (#767842)
Bug 771323 - GParted is showing duplicate mount points for unmounted
encrypted file systems
Mike Fleetwood