GNOME Bugzilla – Bug 689318
filesystem type specific support for partition name maximum length
Last modified: 2013-03-19 16:58:11 UTC
Steps to reproduce: Try to rename a ntfs partition with a name having more than 32 chars. The partition name can't be typed in into the textbox of the renaming dialog, because it has a maximum length of 31 chars, although the filesystem ntfs brings the support for up to 128 chars. Other case, same issue: If you try to rename a partition with a filesystem type that just supports partition names with up to 16 characters, i think it is not very user friendly having a textbox with a limit of 32 chars. When using a longer partition name, the last chars would be truncated and the user wouldn't know in advance, where in his partition name is cut, except he counts. A better chosen maximum for the textbox would give the user some orientation of how long his partition name can be. I think it is more user friendly and also a nice feature to make the maximum length of the renaming dialog (and the new partition dialog) textbox dependent from the filesystem type used. Did some web research for the maximum char lengths of the filesystems that gparted supports to rename (no warranty for correctness): ext{2,3,4}: max 16 chars. http://git.kernel.org/?p=fs/ext2/e2fsprogs.git;a=blob;f=misc/e2label.c;h=477c34bf251d0eba97d28d80c0fe81dd42109a71;hb=d9c56d3ca0bee11e3446ff7e12e3124d28e298a7 http://www.gnu.org/savannah-checkouts/non-gnu/ext2-doc/ext2.html#S-VOLUME-NAME https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout#The_Super_Block ntfs: max (unsigned)(0x100 / sizeof(ntfschar) with typedef u16 ntfschar; and typedef uint16_t u16; so i think: max 256/2=128 characters. http://code.metager.de/source/xref/NTFS-3G/ntfsprogs/ntfslabel.c#265 http://code.metager.de/source/xref/NTFS-3G/include/ntfs-3g/types.h#59 FAT{16,32}: max 11 characters. http://oreilly.com/linux/command-directory/cmd.csp?path=m/mlabel JFS: from `man jfs_tune`: JFS labels can be at most 16 characters long; nilfs: from `man nilfs-tune`: NILFS2 file system labels can be at most 80 bytes long. ReiserFS: from `man reiserfstune`: LABEL can be at most 16 characters long; XFS: from `man xfs_admin`: XFS filesystem labels can be at most 12 characters long; btrfs: max 255. One char of the 256 (the last) terminates the string with 0. from probe.h: #define BTRFS_LABEL_SIZE 256 http://code.metager.de/source/xref/linux/utils/btrfs-progs/btrfslabel.c#62 http://code.metager.de/source/xref/linux/utils/e2fsprogs/lib/blkid/probe.h#63 linux-swap: max 16 chars. from swapheader.h: #define SWAP_LABEL_LENGTH 16 http://code.metager.de/source/xref/linux/utils/util-linux/disk-utils/mkswap.c#198 http://code.metager.de/source/xref/linux/utils/util-linux/include/swapheader.h#152
Thank you for your interest in GParted, and especially for this well researched and detailed bug report. Are you interested in developing a patch to address this problem?
Created attachment 231307 [details] [review] filesystem dependent partition name length
Created attachment 231308 [details] [review] filesystem dependent partition name length - remove comment
Sorry for not answering. Finally found some time to create a patch.
Thank you for the patches. Would you be able to make these into git format patches? http://gparted.org/git.php A patch placed into git format will contain your name, email address, and a comment describing the patch. Otherwise I can look into doing this when I have some time, but I will still need the name and email address for providing you credit for authoring the patch.
Created attachment 231598 [details] [review] git formatted patch
I've formatted the patches with git and concatenated them to one. I did also a small change to one of the comments to place it better. You may credit me as Sinlu Bes.
Hi Sinlu, I noticed that there are few white space errors with the patch. Would you be able to correct them please. 1) Use tabs rather than white spaces to indent the code to match the existing code. 2) Don't add trailing white space to the end of lines. Git complains like this: $ git am bug-689318.patch Applying: Make the partition label length dependent from the file system. /home/mike/programming/c/gparted/.git/rebase-apply/patch:41: trailing whitespace. warning: 1 line adds whitespace errors. It's here: --- a/src/Dialog_Partition_New.cc +++ b/src/Dialog_Partition_New.cc @@ -315,6 +315,11 @@ void Dialog_Partition_New::optionmenu_changed(... >> + + //set partition name entry box length + { + entry .set_max_length( Utils::get_filesystem_label_m... + } 3) Match predominant semicolon style: code, 1 space, semicolon to end each statement. (The existing code is a bit inconsistent and we are changing it one patch at a time). Thanks, Mike
Hi Mike, Thank you for cross-reading my patch so carefully. I didn't read your code guidelines, instead i tried to write like the code that surrounds my changes. I've modified the patch after your suggestions. Greetings, Sinlu
Created attachment 231656 [details] [review] fixed whitespace errors
oh, as i saw on http://gparted.sourceforge.net/development.php, there aren't any code guidelines for gparted. so nothing to have missed to read.
Hi Sinlu, I have now reviewed your code and tested every case. I was pleased to see that you use the file system label length for both changing a label and creating a new file system. My findings are: 1) Add a commented FS_EXFAT case to Utils::get_filesystem_label_maxlength() just like FS_UFS has. We support it to the same level. 2) Change FS_LINUX_SWAP size to 15. mkswap and swaplabel actually truncate the label to this length. 3) Set FS_HFS and FS_HFSPLUS lengths to 255 as mkfs.hfsplus can create file systems with labels up to this length. (Older versions of mkfs.hfsplus with the -h flag can make hfs file systems with the same label limit of 255 characters). 4) Set FS_REISER4 length to 16. mkfs.reiser4 can create file systems with labels up to this size. 5) Remove the now redundant setting of the label input size in Dialog_Partition_New::Set_Data(), line 132: entry .set_max_length( 30 ); 6) Drop curly brackets around line added to Dialog_Partition_New::optionmenu_changed(). Your not introducing a variable and restricting it's scope so it's not necessary: { entry .set_max_length( Utils::get_filesystem_label_maxlength( fs.filesystem ) ) ; } Also a couple of bugzilla tips: 1) When you upload a patch it is useful to mark any obsoleted older versions; 2) You can enter text in the comment box rather than adding a separate comment if you want. Thanks, Mike
Created attachment 232367 [details] [review] Label length extras (v1) Hi Curtis, Would you be able to review the attached patch set. They are a couple of tidy ups I would like to apply after Sinlu's patch. For testing you'll need to apply Sinlu's patch first. Latest is in comment #6. Patch summaries: Avoid reading trailing junk for a reiser4 label (#689318) Remove redundant code trimming labels to length before use (#689318) Thanks, Mike
Created attachment 232377 [details] [review] suggestions of comment #12 Hi Mike, Thank you for testing. I've worked your suggestions into the patch. Greetings, Sinlu
FYI I've submitted this fix upstream: [PATCH] reiser4progs: Limit printing of label to the maximum of 16 chars http://marc.info/?l=reiserfs-devel&m=135679067906237&w=2
Hi Sinlu, Your patch has been committed to the git repository for inclusion in the next release of GParted. I did further testing and noticed additional complexities when labelling hfs, hfs+ and jfs. I adjusted their limits and add comments accordingly. The relevant git commit can be viewed at the following link: Make the partition label length dependent from the file system (#689318) http://git.gnome.org/browse/gparted/commit/?id=ecb1f57594e8aebb60fda3e0160921b83750c54f Thanks, Mike
Thank you Sinlu for enhancing your patch, and Mike for reviewing the patch and committing this with minor enhancements. Mike, the patch in comment #13 looks good. The only comment I have is that the value of 16 characters for reiser4 is included in two places: reiser4::read_label Utils::get_filesystem_label_maxlength It could be argued that the maximum length for a label should be stored with the file system, but since GParted already uses both methods (some things stored in the file system method and some stored in Utils), I am okay with either implementation.
Thank you Mike for committing the patch into git. Greetings, Sinlu
*** Bug 682341 has been marked as a duplicate of this bug. ***
The additional patch in comment #13 has been committed to the git repository for inclusion in the next release of GParted. In addition to the git commit listed in comment #16, the other git commits related to this bug report are: Avoid reading trailing junk for a reiser4 label (#689318) http://git.gnome.org/browse/gparted/commit/?id=18941e24d3cb0b97a6d7cf52800f8f506f6a9ba8 Remove redundant code trimming labels to length before use (#689318) http://git.gnome.org/browse/gparted/commit/?id=d0fec5e26ffae490c1005a19e062d5a1abad0a3d Update AUTHORS http://git.gnome.org/browse/gparted/commit/?id=a6e16ec606fdfac5aa14ee4522c24e2468c43511 Provide credit in About dialog for contributions http://git.gnome.org/browse/gparted/commit/?id=81682eaf2955ca3ec228ff19746b47e3160ca4d9
The code change to address this report has been included in GParted 0.15.0 released on March 19, 2013.