GNOME Bugzilla – Bug 775932
Refactor mostly applying of operations
Last modified: 2017-02-14 18:33:36 UTC
As discussed in bug 774818 comment 5 there is a lot of refactoring needed, mostly to the apply operations code, before LUKS read-write support can be added. This is a separate bug report to capture that refactoring. References: Bug 774818 - Implement LUKS read-write actions NOT requiring a passphrase Bug 627701 - option to encrypt new partitions (using LUKS)
Created attachment 341764 [details] [review] Refactor mostly applying of operations (v1) Hi Curtis, Here's patchset v1 of refactoring applying operations ready for review. There should be no behaviour differences with GParted as a result of these changes. They just make the changes for bug 774818 more focused. (Patchset for bug 774818 is nearly ready itself). Thanks, Mike
Hi Mike, I just finished reviewing patch set v1 from comment #1 (all 5600+ lines). :-) Most every thing looks good to me from a code review perspective, and all the patches apply and compile. The comments are helpful, and you certainly cleaned up some of the more messy areas of the code. So far the only code review thing I found is one minor typo in the commit comment as identified below: P01/20 Refactor Win_GParted::unmount_partition() (#775932) CHANGE FROM: leftover from when the function use to be a separate thread. ^^^ TO: leftover from when the function used to be a separate thread. I have made this change to my copy in anticipation of committing to our git master. Next I need to test much of the functionality of GParted to gain confidence that it still works as expected. Curtis
Hi Mike, Because the patch set touches on many areas, there is a lot to test. :-) I'm tried to test at least one partition/file system with each of the following actions: - Create/delete partition with file system - Mount/umount/swapon/swapoff/activate/deactivate NOTE: Activate/deactivate LVM2 PV do not appear to work, not only with this patch set, but with gparted 0.27.0 as well. This isn't a regression so does not cause a fail in regression testing in my mind. STEPS: 1. Create 256 MiB LVM2 PV (/dev/sdb4) using GParted 2. sudo vgcreate vgtest /dev/sdb4 3. sudo lvcreate --name lvtest --size 128M vgtest 4. Try Partition -> Deactivate - Offline - move/grow/shrink - Online - grow - Copy/Paste/Paste-into-existing - Linux-swap - create/move/grow/shrink/delete - Partition Information - ntfs,ext4,fat32,crypt-luks,lvm2-pv - Whole disk - shrink/check/label/new-uuid So far everything has passed my testing with no regressions in behaviour from the official GParted 0.27.0 release. If I do not find any regressions with subsequent testing then I plan to commit the patch set to our git master tomorrow. Curtis
Hi Curtis, I've just given activate/deactivate LVM PV a go and found it worked for me on CentOS 7 using GParted 0.27.0. I'll need you to debug what is failing for you. Mike
Hi Mike, Since LVM2 PV activate/deactivate works for you I decided to try this on a few distros. The LVM2 PV activate/deactivate actions work properly on the following: fedora 24 VM openSUSE 42.1 VM Ubuntu 12.04 VM Ubuntu 14.04 VM Kubuntu 16.04 physical computer I had done my initial run of tests on an Ubuntu 15.10 (wily) VM. It appears that my Ubuntu 15.10 (wily) VM is the only VM in which LVM2 PV activate/deactivate doesn't work. This leads me to suspect a problem with the VM. Because Ubuntu 15.10 (wily) is no longer supported, I think it might not be worth our time to troubleshoot further. I'll be retiring this VM soon due to loss of support from Canonical. If you have no objections, I will plan to commit the patch set tomorrow. Curtis
Thank you Mike for your continuing efforts to improve GParted. Patch set v1 from comment #1 represents a significant undertaking to refactor the code in anticipation of adding support for LUKS encrypted partition. Patch set v1 has been committed to the git repository for inclusion in the next release of GParted. The relevant git commits can be viewed at the following links: Refactor Win_GParted::unmount_partition() (#775932) https://git.gnome.org/browse/gparted/commit/?id=bfc16bd4a61b96f67703dbe225838c111b8a1999 Refactor activate_mount_partition() and toggle_busy_state() (#775932) https://git.gnome.org/browse/gparted/commit/?id=a5f2c9937b0ef78ed6cbe839720da492f0ab848a Rename file and class to CopyBlocks (#775932) https://git.gnome.org/browse/gparted/commit/?id=fed2595d6dab09499e3e836b78adce24819dd35d Rename copy_filesystem() methods (#775932) https://git.gnome.org/browse/gparted/commit/?id=fd56d7be5782d4c06221d76dcc1f0d2536a65688 Refactor GParted_Core::copy() (#775932) https://git.gnome.org/browse/gparted/commit/?id=0b359a54907c6d04f651820b2def0c6f89ab4ec4 Make encrypted Partition objects look like whole disk device ones (#775932) https://git.gnome.org/browse/gparted/commit/?id=9f08875997651cf19e8ed6cb531afa7412bcfd28 Make set_partition_type() skip whole disk device partitions (#775932) https://git.gnome.org/browse/gparted/commit/?id=829bf0ccc1ac8dd60227d88b2535fab5bf61d68b Make check_repair_filesystem() skip mounted file systems (#775932) https://git.gnome.org/browse/gparted/commit/?id=84acec3f91f7822114894301099fcac1d51ddd86 Make resize_move_partition() skip whole disk device partitions (#775932) https://git.gnome.org/browse/gparted/commit/?id=7caebd2515509022ea8ec4fe505948ab1b758015 Refactor GParted_Core::move() (#775932) https://git.gnome.org/browse/gparted/commit/?id=d2fad35407af2fc57ff2964db14914536c8b173d Properly report specific move and resize errors as bugs (#775932) https://git.gnome.org/browse/gparted/commit/?id=9a1841caaa49109a6f37e7d833718842ebfa4d8a Rename a few GParted_Core apply related methods (#775932) https://git.gnome.org/browse/gparted/commit/?id=0420159c1df34fa532dc12677276e606dcf999a2 Make copying and moving swap depend on mkswap availability (#775932) https://git.gnome.org/browse/gparted/commit/?id=a1c24548563b6dd55531796c0322006af9d8ce5f Refactor resizing file system apply methods (#775932) https://git.gnome.org/browse/gparted/commit/?id=a0158abbeb4b1116c26cfee65bd97c5063e54936 Properly check for file system online resize capabilities (#775932) https://git.gnome.org/browse/gparted/commit/?id=a1c938b30d8ebb556b0dc539f1fc6b5528312a29 Refactor linux-swap recreation (#775932) https://git.gnome.org/browse/gparted/commit/?id=2740113dcb0254c2f9bb171d0572177e213cdb60 Update LVM2 PV activate/deactivate check in set_valid_operations() https://git.gnome.org/browse/gparted/commit/?id=683b4da0e45abf66bb5fe1da926647aaf6003335 Remove "../include/" from GParted header #includes https://git.gnome.org/browse/gparted/commit/?id=8979913a3f89f0593b4efba5c9d6781f565eb3d6 Stop providing a default for FileSystem::resize() fill_partition argument https://git.gnome.org/browse/gparted/commit/?id=029a8eb19d9cc6fbd8dfa685bb5972bc58c65fb5 Improve comment about disallowing shrinking when the FS usage is unknown https://git.gnome.org/browse/gparted/commit/?id=8373d36ed26b110bd11b04fc354f6bee49dfb9b5 Curtis
Created attachment 342085 [details] [review] Add missing shrink file system message (v1) Hi Curtis, While testing the LUKS patchset I found a small bug in patchset v1. Here is the fix for it. Thanks, Mike
Hi Mike, Patch v1 from comment #7 looks good to me and passed my testing. The "add missing shrink file system message (v1)" patch has been committed to the git repository. The relevant git commit can be viewed at the following link: Add lost "shrink file system" operation detail message (#775932) https://git.gnome.org/browse/gparted/commit/?id=c03c5d11dc505397bf36a33976c29df43e08f29a Curtis
This enhancement was included in the GParted 0.28.0 release on February 14, 2017.