After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 695396 - Please apply f2fs patch
Please apply f2fs patch
Status: RESOLVED FIXED
Product: gparted
Classification: Other
Component: application
GIT HEAD
Other Linux
: Normal enhancement
: ---
Assigned To: gparted maintainers alias
gparted maintainers alias
Depends on:
Blocks:
 
 
Reported: 2013-03-07 20:19 UTC by Patrick Verner
Modified: 2013-04-24 16:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to add f2fs support to GParted (7.53 KB, patch)
2013-03-15 16:50 UTC, Patrick Verner
needs-work Details | Review
Add f2fs file system support patch v2 (7.79 KB, patch)
2013-03-16 19:29 UTC, Patrick Verner
needs-work Details | Review
Add f2fs file system support patch v3 (8.14 KB, patch)
2013-03-16 21:11 UTC, Patrick Verner
none Details | Review

Description Patrick Verner 2013-03-07 20:19:07 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/
Comment 1 Mike Fleetwood 2013-03-09 10:30:56 UTC
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
Comment 2 Mike Fleetwood 2013-03-11 23:14:52 UTC
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
Comment 3 Patrick Verner 2013-03-13 03:22:54 UTC
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. :)
Comment 4 Patrick Verner 2013-03-13 16:54:35 UTC
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.
Comment 5 Mike Fleetwood 2013-03-14 02:23:54 UTC
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
Comment 6 Mike Fleetwood 2013-03-14 02:35:45 UTC
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.
Comment 7 Patrick Verner 2013-03-15 16:50:39 UTC
Created attachment 238991 [details] [review]
Patch to add f2fs support to GParted
Comment 8 Patrick Verner 2013-03-15 16:51:21 UTC
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.
Comment 9 Mike Fleetwood 2013-03-16 17:27:53 UTC
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.
Comment 10 Patrick Verner 2013-03-16 19:26:41 UTC
Review of attachment 238991 [details] [review]:

f2fs-v2.mbox obsoletes this patch.
Comment 11 Patrick Verner 2013-03-16 19:29:03 UTC
Created attachment 239035 [details] [review]
Add f2fs file system support patch v2

Here is my second attempt... :)
Comment 12 Patrick Verner 2013-03-16 21:10:25 UTC
Review of attachment 239035 [details] [review]:

Missed the label comment about 16 characters.
Comment 13 Patrick Verner 2013-03-16 21:11:56 UTC
Created attachment 239038 [details] [review]
Add f2fs file system support patch v3

Hopefully the third time is the charm.
Comment 14 Mike Fleetwood 2013-03-19 22:17:42 UTC
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
Comment 15 Curtis Gedak 2013-03-24 19:07:35 UTC
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
Comment 16 Curtis Gedak 2013-04-24 16:28:37 UTC
The code change to address this report has been included in GParted 0.16.0 released on April 24, 2013.