GNOME Bugzilla – Bug 438573
cancel out overlapping actions
Last modified: 2011-11-01 17:09:28 UTC
For example, if I want to grow partition A to size 10GB, and then I change my mind, and want to grow it to size 20 GB, it is not: * grow to 10GB * grow to 20GB it is simply * grow to 20GB
*** Bug 625249 has been marked as a duplicate of this bug. ***
Same problem on gparted 0.5.1 on KUbuntu 10.04.
the action should not be added to the action list until the 'resize/move' dialogue is closed. or instead of an action list * grow by x * move left by x * shrink by x * move by x it should just be a list of boundary changes. * set end to x * set start to x * set end to x ... which can be interpreted into grows shrinks and moves when you click apply. also maybe trivial moves could be warned against, and optionally dropped. eg if you try to move a 100GB partition by a few MB.
Created attachment 194751 [details] [review] Patch file Patch generated using "git diff > changes.diff"
This bug has been seen on version 0.7.0 Please find in attachment a fix for this bug. This is a "git diff" patch relative to git version 786162146c90ac570ef9d577758d7c5f477010f5 The patch consist of the following. When a new operation is added to operations list, we check if a merge is possible depending on the operation type : - OPERATION_RESIZE_MOVE : 2 consecutive "resize" operations on the same partition - OPERATION_LABEL_PARTITION : 2 "label change" operations (need not be consecutive) on the same partition - OPERATION_CHECK : 2 "check" operations (need not be consecutive) on the same partition - OPERATION_FORMAT : 2 consecutive "format" operations on the same partition Feel free to test out this patch.
Hi Jérôme, thank you for this patch. I have just started to investigate this patch and test the first listed feature whereby 2 consecutive "resize" operations on the same partition are merged. While testing I discovered some unusual behaviour. After a few resize only operations, the dialog that warns about a move operations popped up. Since I did not change the space preceding value this should not have been displayed. I continued doing some more resize operations and encountered a segmentation fault. I tried to reproduce the behaviour, but was unable to hit the segmentation fault, only the incorrect dialog box. Here are the steps I recently tried. 1) Create a 4096 MiB ext4 partition and apply the operation. 2) Resize partition to 8192 MiB, click Resize/Move, now 1 operation pending 3) Resize partition to 10000 MiB, click Resize/Move, "Moving a partition..." dialog appears. This should not happen since the "free space preceding" value has not been changed". Click "OK" 1 operation pending 4) Resize partition to 4096 MiB, click Resize Move, now 1 operation pending 5) Resize partition to 6000 MiB, click Resize/Move, now 1 operating pending 6) Resize partition to 10000 MiB, click Resize/Move, now 1 operation pending 7) Resize partition to 8192 MiB, click Resize/Move, "Moving a partition..." dialog appears. Again, this should not happen since the "free space preceding" value has not been changed". Click "OK" 1 operation pending 8) Resize partitoin to 6000 MiB, click Resize/Move, now 1 operation pending 9) Resize partition to 6144 MiB, click Resize/Move, now 1 operation pending 10) Resize partition to 12000 MiB, click Resize/Move, "Moving a partition..." dialog appears. Again, this should not happen. Click "OK" 1 operation pending 11) Resize partition to 1024 MiB, click Resize/Move, "Moving a partition..." dialog appears. Again, in theory since I did not change the "free space preceding" value there should be no move involved. Click "OK" 1 operation pending 12) Resize partition to 2048 MiB, click Resize/Move, now 1 operation pending. Do you experience similar behaviour?
Okay, now it appears that I can reproduce the segmentation fault with a variation of the following steps. 1) Start GParted with a pre-existing 2048 MiB ext4 partition (from when I applied step 12 above). 2) Resize partition to 10000 MiB, click Resize/Move, now 1 operation pending 3) Resize partition to 20000 MiB, click Resize/Move, segmentation fault! Sometimes one more step is required: 4) Resize partition to 10000 MiB, click Resize/Move, segmentation fault! Each time I am typing in a new number in the "New Size" value, clicking on the "Free space after" value to activate the "Resize/Move" button, then clicking on the "Resize/Move" button.
Created attachment 197111 [details] [review] Generated using 'git diff > changes_2.diff' Hi Curtis, I have reproduced the two issues you mention and think they are only one problem. The problem is that the test if we need to show the "Moving a partition..." dialog is done after the merge and, consequently, we try to access some previously deleted data (when the merge is successfull, the new operation is freed). This sometimes goes to segmentation fault if the freed memory has been released to the system. Or goes to unexpected values if the freed memory has been used to something else. I think I've solved this issue putting the merge after the test for the dialog. You can test this patch instead of the previous one.
Created attachment 197170 [details] GParted Screenshot - Multiple label operations problem Thank you Jérôme for your prompt response to the segmentation fault problem. I have re-tested using the steps in comment #6 and comment #7 and have not re-encountered the problems previously identified. Hence I can conclude that your updated diff patch fixes these problems. :-) Next I tested the changes to OPERATION_LABEL_PARTITION. Unfortunately I encountered two problems that are shown in the attached screen shot: 1) When changing the label, the "View Operations" portion of the window does not display the new label name. Instead a label of "1" is shown. This seems to be a display only problem because when I apply the label operation the correct label is written. 2) When I set and re-set the label on two partitoins multiple times, the visual representation of the disk was not refreshed, and in fact went completely grey. This is also shown in the attached screen shot. I have tried to create a short sequence of steps that will reproduce this behaviour, but I have been unsuccessful in this endeavour. If/when I have a series of steps to reproduce this problem then I will post these too. Assuming you can reproduce these problems and wish to fix these, then would you also rename the method "merge_operations" to "Merge_Operations"? This would better match with the method names in this portion of the code.
Problem number 1 listed in comment #9 does not appear to be related to the git.diff patch. I just confirmed that the problem also exists on our recent GParted 0.9.1 release. I will dig into this label issue further to try to find the root cause.
Created attachment 197270 [details] [review] Patch file (Version 3) Damn !!! It seems that this patch introduces more problems than it solved :) !!! So, I tried to reproduce those 2 problems. 1) I tried a lot of times to change the label of ext2 and ext4 partitions, but I can't fall into the "label of 1" issue you mention. Have you got a sequence of steps to reproduce it ?? 2) When I first implemented this patch, I encountered this problem : a grey pane appears instead of visual representation of the disk (as depicted on your screenshot). But If I remember correctly, this was not on OPERATION_LABEL_PARTITION operation. But unfortunatly, I tried to reproduce it now... but all my attempt where unsuccessfully. Again, have you got a sequence of steps to reproduce it ?? Nevertheless, I tried a patch to the issue #2. Tell me if it change something. I also renamed the method "merge_operations" to "Merge_Operations".
I have traced the cause of problem #1 in comment #9 to an error in the Canadian English translation file. This file was updated in GParted 0.9.1, and because I happen to be using Canadian English I stumbled upon this problem. I have created a bug report and submitted a patch to the translation team: Bug #660180 - GParted parameter missing percent sign in en_CA translation https://bugzilla.gnome.org/show_bug.cgi?id=660180 Please accept my apologies for suggesting your patch as a cause this problem. I will use your updated patch file (version 3) to test if I can reproduce problem #2 in comment #9. If I cannot reproduce the problem then I will move on to test the other functionality added by the patch.
Jérôme, I seem to have discovered a set of steps that reproduce the disappearing visual disk graphic. I do not yet know if the cause is the code from the patch, or if this is a pre-existing problem. Following are the steps to reproduce problem 2 from comment #9. -------------------------------------------------------------- For this test, two disk devices are required. Drive A needs at least two partitions that can be labeled. Drive B needs at least one partition that can be labeled. 1) On Drive A, re-label an existing partition (a1). 2) Select Drive B from the drop down list. 3) On Drive B, re-label an existing partition (b1). 4) Select Drive A from the drop down list. 5) On Drive A, re-label the previously re-labeled partition (a1). 6) On Drive A, re-label a different existing partition (a2). 7) On Drive A, re-label the same existing partition (a2). At this point the visual disk graphic display disappears. Are you able to reproduce the graphic disappearing problem?
Created attachment 197706 [details] [review] Version 4 Hi Curtis, Yes, I am able to reproduce the graphic disappearing problem with the sequence of steps you gave. After a quick look, I think it's comming from the order of GTK graphical events in the event loop (resize, redraw, ...). Or maybe by a missing redraw event. So I added a redraw event after the resize one and it seems to fix this issue. So there is a new patch. And I think this one is the good one :-) !!!
Hi Jérôme, Your latest patch seems to address the visual disk graphic disappearing problem. :-) I will use this latest patch and continue testing the new enhancements offered by the patch. It might be next week before I can focus again on more testing. I will continue to keep you up-to-date on the testing results. Thanks again for your prompt fixes to the problems we have found.
My testing of the last two enhancements enabled by this patch has gone well. :-) Before I commit this patch to the git repository I plan to re-test the resize and label enhancements, and also to review the source code one more time. Jérôme, are there any scenarios that you can think of that might cause problems that I should test?
No, Curtis, I don't see any test cases that might cause problems...
This patch has been committed to the gnome git repository for inclusion in the next release of GParted. Jérôme, I took the liberty of adding an "else" in front of the last "if" statement in the Win_GParted::Merge_Operations(...) method. I also made some minor formatting changes before committing your patch. The relevant git commit can be viewed at the following link: http://git.gnome.org/browse/gparted/commit/?id=b10349ae37df06b7bf7de91ea93a3913535ade3a Thank you again Jérôme for this patch to enhance GParted. Your contribution is much appreciated. Sincerely, Curtis Gedak
Yes, Curtis, you're right, the if/else statement you corrected was a mistake. I've seen you added my name in AUTHORS file, thx :) You welcome Jérôme
(In reply to comment #18) I have looked a bit at the addition "bool Win_GParted::Merge_Operations(int first, int second)". http://git.gnome.org/browse/gparted/diff/src/Win_GParted.cc?id=b10349ae37df06b7bf7de91ea93a3913535ade3a I wonder if this member function does it really need to allow that the passed parameters can be negative integers. Would you like to enforce that only non-negative values will be used for indexes?
Created attachment 198653 [details] [review] Changed parameters type Yes, I agree with you Markus. There is no need to have int parameters. I also add some checks on parameters to be less than operations.size() to prevent indexing the operation vector outside of its boundary. A new patch is attached.
(In reply to comment #21) Thanks for your acceptance. Is a new bug report needed to track similar adjustments for the safe handling of index variables in other places of the source code base?
I reviewed the whole Win_GParted.cc file. I didn't find any similar adjustements to make. For the rest of the source code base, I think it would be a huge work to do ;-) !!!
Thank you Jérôme and Markus for these code improvements. These enhancements have been committed to the GNOME repository and can be viewed at the following link: http://git.gnome.org/browse/gparted/commit/?id=03cdaed9461feed602536df0a02ab34ec0fa1cf1 (In reply to comment #22) > Is a new bug report needed to track similar adjustments for the safe handling > of index variables in other places of the source code base? If you find similar adjustments or improvements, then please open a separate bug report and attach the patch that implements the improvements.
Thank you again Jérôme for this enhancement patch. The enhancements to address this bug report were included in the GParted 0.10.0 release on November 1, 2011.