GNOME Bugzilla – Bug 695396
Please apply f2fs patch
Last modified: 2013-04-24 16:28:37 UTC
This is a patch that adds f2fs support to GParted. f2fs-tools only supplies mkfs.f2fs at this time. Patch location: http://beefdrapes.partedmagic.com/source/gparted/ Test version of Parted Magic that includes this patch: http://beefdrapes.partedmagic.com/
Hi Patrick, Thank you for the patch. I have performed an initial review and the only minor recommendation is that you should use your name and the current year in the copyright line at the top of newly created files. At the moment we are in the process of stablising a sizeable patch set [1] ready for the next release of GParted. Therefore it's likely to be several weeks before we can add this patch. Also at that time the patch will need rebasing because patch set [1] and at least one other queued patch set [2] will have changed code this patch touches. Either you can update the patch or I can do it on your behalf. Finally to give full credit for the change can you let us know your email address or send a git formatted patch containing your email address. If you don't want to do that's OK too. Your patch is available from my GIT repo at: https://rockover.homeip.net/cgit/gparted/ Branch: f2fs-review Thanks, Mike -- [1] Bug 685740 - Refactor to use asynchronous command execution https://git.gnome.org/browse/gparted/ Branch: psusi/refactor [2] Bug 688882 - Improve clearing of file system signatures https://rockover.homeip.net/cgit/gparted/ Branch: erase-v1
Have to updated the colour for f2fs from a deep pink to a accent red from the GNOME palette for aesthetic reasons. Intermediate commit added to f2fs-review branch: https://rockover.homeip.net/cgit/gparted/commit/?h=f2fs-review&id=919c9573b66492f388d14d2750e775ccb0ee6b46
I cloned git today and noticed the patch was pretty well killed with the new changes. I'll get to work on a new one tomorrow. Although the pink was pretty, I have to agree it was outside of the gnome palette. :)
The f2fs patch is updated. Same place. http://beefdrapes.partedmagic.com/source/gparted/ I'm not a very good C programmer, but I would like to supply git patches like you suggested. If you want I could maintain f2fs in GParted for the foreseeable future (if it's added). They don't have very many tools yet and more updates will be needed when they are added. The problem is I have no idea how to do it or exactly how you want them submitted. Some detailed in instructions would be helpful or I can just let you know when I have new patches over at my site.
The normal way of working is to attach patches to bug reports. Git formatted patches have multiple advantages: records your authorship, single file can contain multiple patches, easier to apply. I've produced a first draft of "How to create your first GParted patch using git" in this forum thread: http://gparted-forum.surf4.info/viewtopic.php?id=16820 Also here are a few links to get you going with git. (1) to (3) are worth reading and watching. Use (4) as a reference. 1) "Getting the most out of Git in GNOME" https://live.gnome.org/Git/Developers 2) "HOWTO: Create and submit your first Linux kernel patch using GIT" http://linux.koolsolutions.com/2011/02/26/howto-create-and-submit-your-first-linux-kernel-patch/ 3) "Git For ages 4 and up" video from LCA 2013 http://mirror.linux.org.au/linux.conf.au/2013/mp4/Git_For_Ages_4_And_Up.mp4 4) "Pro Git" book online http://git-scm.com/book
I was having trouble testing this on my own machine but I have finally discovered that the mkfs.f2fs command packaged on Fedora 17 has an old copy of the code which writes the super block at the wrong offset. Therefore blkid from util-lixux and GParted weren't able to recognise the f2fs file systems it was creating. Anyway sorted now.
Created attachment 238991 [details] [review] Patch to add f2fs support to GParted
I hate keep bothering you, but I have read your instructions and have attempted to submit a git patch. Let me know if this patch is acceptable. If it is I'll submit them like this in the future.
It's not a problem to answer your questions. What you've done works, but following are a few improvements. (Either I can do these for you when I commit the code upstream, or you can upload a newer patch. Also we have frozen the code for translations so I can't commit until after Tuesday when the next release of GParted is due). Review comments: 1) Doesn't set f2fs maximum label size. Utils::get_filesystem_label_maxlength() (Sorry didn't spot first time around. Also took some testing with mkfs.f2fs. Claims support for 512 char labels. Crashes with labels >= 29! Suspect writes corrupted labels > 19 characters because blkid can't read them longer than 19. Therefore use 19 characters as the maximum label size). 2) Doesn't mention the new f2fs-tools in the README file. (Sorry didn't mention this before). 3) Applying the patch reports a number of white space mistakes. Utils.cc - You've added tabs at the end of two of the lines you've added. f2fs.cc - Possibly as a result of copying another source file there are several lines in f2fs::get_filesystem_support() which are blank but contain a tab character. $ git am f2fs-v2.patch Applying: Patch to add f2fs support /home/mike/programming/c/gparted/.git/rebase-apply/patch:185: trailing whitespace. case FS_F2FS : return "f2fs" ; /home/mike/programming/c/gparted/.git/rebase-apply/patch:193: trailing whitespace. case FS_F2FS : return "f2fs-tools" ; /home/mike/programming/c/gparted/.git/rebase-apply/patch:229: trailing whitespace. /home/mike/programming/c/gparted/.git/rebase-apply/patch:231: trailing whitespace. /home/mike/programming/c/gparted/.git/rebase-apply/patch:234: trailing whitespace. warning: squelched 2 whitespace errors warning: 7 lines add whitespace errors. 4) Poor coding indentation. f2fs::get_filesystem_support() Missing indent for conditional statement after if. if ( ! Glib::find_program_in_path( "mkfs.f2fs" ) .empty() ) >> fs .create = GParted::FS::EXTERNAL ; 5) Missing bug number from patch description. We include Bugzilla bug numbers in patch summaries so that we can refer back from a change in the code to the bug. This works really well because when browsing the git repository on the GNOME web site at https://git.gnome.org/browse/gparted/ it converts Bug 695396 into a clickable links, just like bugzilla has done here. Also as GParted supports a wide range of distributions with different (versions of) support tools we like to mention those tools and any specific versions needed when making changes. I suggest this for the commit message: --8<-- Add f2fs file system support (#695396) Only supports detection and creation of f2fs file systems. Requires f2fs-tools and a blkid with f2fs support, util-linux > 2.22.2. f2fs-tools v1.1.0 only supports file system creation. Currently requires util-linux directly from the git repository as f2fs support was only committed on 5 Feb 2013 and it has not yet been released. Closes Bug #695396 - Please apply f2fs patch --8<-- Couple of bugzilla hints: 1) You can enter comments at the same time as uploading an attachment. 2) When uploading a newer version of a patch mark previous versions as superseded.
Review of attachment 238991 [details] [review]: f2fs-v2.mbox obsoletes this patch.
Created attachment 239035 [details] [review] Add f2fs file system support patch v2 Here is my second attempt... :)
Review of attachment 239035 [details] [review]: Missed the label comment about 16 characters.
Created attachment 239038 [details] [review] Add f2fs file system support patch v3 Hopefully the third time is the charm.
Hi Patrick, Thanks for the patch. I took the liberty of updating the README file, adding a comment and adjusting some indentation. I have added you to the AUTHORS file to give you credit. The patch has been committed to the git repository for inclusion in the next release of GParted. The relevant git commit: Add f2fs file system support (#695396) https://git.gnome.org/browse/gparted/commit/?id=9b649ed445652dea9738d15cd5ad71864c14761b Thanks, Mike
Thank you Patrick for creating this patch, and thank you Mike for shepherding this patch into the git repository. Patrick, In another bug report you expressed an interest in getting more familiar with the GParted code. We have some general instructions on how development is handled in the following page: http://gparted.org/development.php With your recent patch, the number of file systems supported is increasing. One challenge introduced by this increase is that the "View --> File System Support" dialog window is becoming quite long. This can cause a problem with smaller displays (e.g., 800x600). Perhaps you might want to investigate enhancing this window to be resizable with scrollbars? Depending how far you would like to pursue this, there is an existing open bug report regarding "too much information in this features dialog". Bug #342682 - too much information in 'features' dialog
The code change to address this report has been included in GParted 0.16.0 released on April 24, 2013.