GNOME Bugzilla – Bug 683149
Cleanup(?): Remove lp_device and lp_disk from GParted_Core
Last modified: 2012-12-12 18:13:08 UTC
Created attachment 223127 [details] [review] Remove lp_device and lp_disk from GParted_Core Hi, I'm new to this project, and I felt that lp_device and lp_disk members of GParted_Core where used like global variables. This makes understanding of the code unnecessarily hard. I thus wrote a patch to move those variables to the functions where they are actually defined/add them to the parameter list of functions using them. Comments are appreciated!
Created attachment 223130 [details] [review] Remove lp_partition from GParted_Core Like the first patch, but removes lp_partition from GParted_Core
Created attachment 223132 [details] [review] Remove GParted_Core::temp
Created attachment 223133 [details] [review] Remove GParted_Core::partition_temp
Created attachment 223134 [details] [review] Remove GParted_Core::fs
Created attachment 223135 [details] [review] Remove GParted_Core::p_filesystem
Those patches will also necessary to make GParted_Core::set_device_partitions static so it can be used in the LUKS module to build the Partitions inside the LUKS container.
Hi Matthias, Thank you for creating this bug report to improve the GParted code. I agree that using global variables makes understanding, and hence maintaining the code more difficult. Since you mentioned that these patches are needed for the LUKS support enhancement, I will plan to review these changes first. Regards, Curtis
Created attachment 223170 [details] [review] Cleanup, rebased on master
As those changes are already in the LUKS bug, I mark this bug as duplicate and work on (i.e. rebase) those changes there. Otherwise I would just post the same patchset here and in the LUKS bug. *** This bug has been marked as a duplicate of bug 627701 ***
Hi Matthias, Firstly your absolutely correct that using member variables like global variables is not good practice and makes understanding the life of data difficult to understand. Review findings: 1) Please make every commit compile. It is important for troubleshooting using git bisect. Commit 3 to 6 inclusive failed to compile for me. > Subject: [PATCH 03/23] Remove GParted_Core::temp $ git checkout 42aafec36e780b7febae74e2d2442347e09edfe1 HEAD is now at 42aafec... Remove GParted_Core::temp $ make GParted_Core.cc: In member function ‘GParted::FILESYSTEM GParted::GParted_Core::get_filesystem(PedDevice*, PedPartition*)’: GParted_Core.cc:1164:4: error: ‘temp’ was not declared in this scope make: *** [GParted_Core.o] Error 1 2) This looks like a bug fix rather that just code refactoring but I'm not sure. Please confirm. > Subject: [PATCH 01/23] Remove lp_device and lp_disk from GParted_Core ... > @@ -242,7 +240,6 @@ void GParted_Core::set_devices( std::vector<Device> & devices ) > } > lp_device = ped_device_get_next( lp_device ) ; > } > - close_device_and_disk() ; > > std::sort( device_paths .begin(), device_paths .end() ) ; > } 3) Please update the 6 commit messages to reference bug 683149 like this. Please see any of the code changes committed in the git repository over the last few months for examples. It goes like this: One line short summary (#nnnnnn) Optional longer description on why the change is necessary. For fixing bugs including a test case and error message is useful. For non-obvious changes explain why it was fixed in a particular way and provide an overview to understand the change. Bug nnnnnn - Bug summary line from GNOME bugzilla 4) Please use tabs and spaces when indenting function parameters when continuing on following lines. Tabs to the start of the previous line, then spaces. Means those of us using the default tab size of 8 characters don't have the continuation lines starting at the end of the line. Do it like this: Legend: "_" represents tab, "." represents space __myfunction( param1, __............param2 ) ; Just one example: > Subject: [PATCH 01/23] Remove lp_device and lp_disk from GParted_Core ... > +bool GParted_Core::open_device_and_disk( const Glib::ustring & device_path, > + PedDevice*& lp_device, PedDisk*& lp_disk, bool strict ) 5) GParted_Core::get_fs() seems to still use member variable GParted_Core::fs after patch number 5 removed it. > Subject: [PATCH 05/23] Remove GParted_Core::fs ... > @@ -214,7 +214,6 @@ private: > std::map< FILESYSTEM, FileSystem * > FILESYSTEM_MAP ; > FileSystem * p_filesystem ; > std::vector<PedPartitionFlag> flags; > - FS fs ; > std::vector<Glib::ustring> device_paths ; > bool probe_devices ; > Glib::ustring thread_status_message; //Used to pass data to show_pulsebar method 6) Please remove these extra curly braces which have been added to only some sites in patch number 6 and aren't needed. > Subject: [PATCH 06/23] Remove GParted_Core::p_filesystem ... > @@ -1335,9 +1334,12 @@ void GParted_Core::read_label( Partition & partition ) > switch( get_fs( partition .filesystem ) .read_label ) > { > case FS::EXTERNAL: > - if ( set_proper_filesystem( partition .filesystem ) ) > + { Here > + FileSystem* p_filesystem = set_proper_filesystem( partition .filesystem ); > + if ( p_filesystem ) > p_filesystem ->read_label( partition ) ; > break ; > + } and here > #ifndef HAVE_LIBPARTED_3_0_0_PLUS > case FS::LIBPARTED: > break ; Thanks, Mike
Created attachment 227622 [details] [review] Cleanup v3 Thanks for your comments. Hi, This patch is - rebased against current master - every commit comiles on its own - all commit messages fixed - fixes are split from refactoring - tabs/spaces fixed I don't see GParted_Core::get_fs() using the global fs after the patch (and it compiles). It does use a local static variable, but my patch didn't touch this. The curly braces are needed. Otherwise the compiler will complain "jump to label crosses initialization of p_filesystem". Thanks, Matthias
Hi Matthias, (In reply to comment #11) > This patch is > - rebased against current master > - every commit comiles on its own > - all commit messages fixed > - fixes are split from refactoring > - tabs/spaces fixed OK > I don't see GParted_Core::get_fs() using the global fs after the patch (and it > compiles). It does use a local static variable, but my patch didn't touch this. Finding (5) pass. I missed the "static FS fs" and didn't notice it was a local variable hiding member variable of the same name. > The curly braces are needed. Otherwise the compiler will complain "jump to > label crosses initialization of p_filesystem". Finding (6) pass. Sorry about that I've just learnt some more about variable scoping in case statements. New finding: 7) Changes to LP_set_used_sectors() leads to a core dump reading used sectors of hfsplus file systems. Patch number 1 add a call to open_device_and_disk() into LP_set_used_sectors() where previously it just used the already open disk using the now removed member variable. > @@ -1616,7 +1626,9 @@ void GParted_Core::LP_set_used_sectors( Partition & partition ) > PedFileSystem *fs = NULL; > PedConstraint *constraint = NULL; > > - if ( lp_disk ) > + PedDevice* lp_device = NULL ; > + PedDisk* lp_disk = NULL ; > + if ( open_device_and_disk( partition .device_path, lp_device, lp_disk ) ) > { > lp_partition = ped_disk_get_partition_by_sector( lp_disk, partition .get_sector() ) ; Patch number 3 adds the matching close_device_and_disk() into LP_set_used_sectors(), but claims to be a bug fix. > @@ -1648,6 +1648,7 @@ void GParted_Core::LP_set_used_sectors( Partition & partition ) > ped_file_system_close( fs ) ; > } > } > + close_device_and_disk( lp_device, lp_disk ) ; > } > } > #endif Thanks, Mike
Created attachment 227926 [details] [review] Cleanup v4 Thanks for finding that bug! I fixed it in the current patchset by supplying the lp_disk as a parameter instead of reopening it. I cannot test the LP_set_used_sectors, because with my libparted HAVE_LIBPARTED_FS_RESIZE evaluates to false. But it looks right to me now.
Created attachment 228427 [details] [review] Cleanup v5 Hi Matthias and Curtis, Patchset "Cleanup v4" from comment #13 passed review and works. However, my perfectionism has kicking in so rather than make you jump through excessive hoops I have made the following updates to the patchset. Please confirm OK, then I'll commit. 1) Made all changes comply with the predominate GParted white space style. 2) Revise patch number 1 so that open_device_and_disk() still calls open_device() rather than calling ped_device_get() directly so that the patch remains a strictly refactoring only one. 3) Improve comment for patch number 2 to explain why removal of close_device_and_disk() is safe. Plus remove header and footer as it's a cleanup outside the scope of the bug description. 4) Re-order parameters to commit_to_os() to be(lp_disk, timeout) as it's more logical to me. 5) Add missing # to commit message in footer like "Bug #nnnnnn" after I advised you wrong. 6) In patch number 7 some p_filesystem variables needed curly braces around to limit variable scope in case statements, others didn't. Define variable above switch statement everywhere and remove curly braces to be consistent. Thanks, Mike
The changes look good to me.
Mike and Matthias, This is a heads up that I've been buried in book editing work today and likely will be for the next few days. Hence it might be a few days before I can get to reviewing this. Your patience is appreciated.
Hi Mike and Matthias, Great work! This 4th patch set looks good. For testing I used an ext2 file system, and ran through create, delete, resize, move, copy, paste, label, and set new UUID, and these all ran well. Mike, you have my okay to commit this patch set. We'll keep the bug open until this enhancement is included in an official release tarball. Thanks again, Curtis
Hi Matthias, Patchset "Cleanup v5" from comment #14 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: Remove lp_device and lp_disk from GParted_Core (#683149) http://git.gnome.org/browse/gparted/commit/?id=500f1bcd9bdaa37a4efd214e102021295a3cee5c Remove unnecessary close_device_and_disk() http://git.gnome.org/browse/gparted/commit/?id=7dd46df954cd8e380b671bad31d010f54aef70d6 Remove lp_partition from GParted_Core (#683149) http://git.gnome.org/browse/gparted/commit/?id=6a3b17adc03e337bd86d5397b0f105d23225a756 Remove GParted_Core::temp (#683149) http://git.gnome.org/browse/gparted/commit/?id=a13bed642449278f17f787c31ef1c2af051b8d3a Remove GParted_Core::partition_temp (#683149) http://git.gnome.org/browse/gparted/commit/?id=d2ced083551c1973ebd770ce4afcbb56275d6c1c Remove GParted_Core::fs (#683149) http://git.gnome.org/browse/gparted/commit/?id=52af838df2a18a0cbf7f749f0baeaff05fd7d0a6 Remove GParted_Core::p_filesystem (#683149) http://git.gnome.org/browse/gparted/commit/?id=1f3b11748e99c0b56fed35a3fc77b2da5dc3072e Thanks, Mike
Thanks Mike for the clarification. I did mean "Cleanup v5", and not 4 as I mistakenly referred to in comment #17.
Thanks, I'll rebase the luks patchset in the near future.
The enhancements to address this bug report have been included in GParted 0.14.1 released on December 12, 2012.