GNOME Bugzilla – Bug 701569
Add create_with_label flag to FS struct
Last modified: 2013-09-18 16:55:10 UTC
Classes for hfs and hfsplus contain lines like this: if ( ! Glib::find_program_in_path( "vol_id" ) .empty() ) fs .read_label = FS::EXTERNAL ; (method hfs::get_filesystem_support()) https://git.gnome.org/browse/gparted/tree/src/hfs.cc?id=GPARTED_0_16_1#n42 This means that they only use vol_id. On Ubuntu >= 10.04, vol_id is deprecated, and can't be invoked anymore, and on most recent Ubuntu versions (tested on 13.04) it isn't even in package repos anymore. blkid is available on Ubuntu can do exactly the same as vol_id, so GParted should use blkid as fallback, like it does already in FS_Info.cc: https://git.gnome.org/browse/gparted/tree/src/FS_Info.cc?id=GPARTED_0_16_1#n71
Created attachment 245970 [details] [review] patch proposal proposed patch
Hi Sinlu, From my testing with of GParted 0.16.1 on Ubuntu 13.04, which does not have vol_id, GParted does use blkid to determine the hfs and hfs+ volume names. The code to do a graceful "fall back" to blkid for all file systems is contained in the GParted::set_device_partitions method. See: https://git.gnome.org/browse/gparted/tree/src/GParted_Core.cc?id=GPARTED_0_16_1#n1119 Is this not working on your test machine? Curtis
Hi Curtis, Reading itself works on my machine. But the read_label attribute isn't set, as I doesn't have vol_id in my path. This doesn't affect usability of GParted but its an internal flaw, which can affect further improvement of GParted. Background: It caused me doubts in my work for Bug 691681, while I was testing support for filesystems which can only be labeled on reformatting (I haven't posted the code yet / the posted code has errors). As reiser4 doesn't work on my machine: mkfs.reiser4 --yes --label "" /dev/sda7 Warn : Linux 3.8.0-22-generic is detected. Reiser4 does not support such a platform. Use -f to force over. My only alternative therefore was hfs/hfsplus. My code used the read_label attribute to determine whether labeling on reformat can be done or not. See bug 691681 comment 10 for behaviour you proposed, it makes use of read_label. A question to understand GParted code better: Why is the method hfs::read_label needed in the first place, as that functionality is already given by GParted::set_device_partitions ? Sinlu
Hi Sinlu, > Reading itself works on my machine. > But the read_label attribute isn't set, as I doesn't have vol_id in my path. > This doesn't affect usability of GParted but its an internal flaw, which can > affect further improvement of GParted. Thank you for clarifying this in terms of the read_label ability. If I recall correctly, this read_label ability is only used to test if the specific file system method has a command to read the file system label. > mkfs.reiser4 --yes --label "" /dev/sda7 > Warn : Linux 3.8.0-22-generic is detected. Reiser4 does not support such a > platform. Use -f to force over. Reiser4 is not supported on Linux 3.x kernels. See: Bug #688523 - Can't format 'reiser4' > My only alternative therefore was hfs/hfsplus. My code used the read_label > attribute to determine whether labeling on reformat can be done or not. > See bug 691681 comment 10 for behaviour you proposed, it makes use of > read_label. Now I understand. This is where the challenge arises. :-) > A question to understand GParted code better: Why is the method hfs::read_label > needed in the first place, as that functionality is already given by > GParted::set_device_partitions ? Following is a bit of history: - hfs::read_label command was added back in 2008 (0.3.6) when I added volume label support to GParted. - the use of blkid to read labels was added later that year (0.4.0) to improve performance (if blkid could not read the label, then use FS specific read_label) - in 2011 (0.11.0) a problem with blkid reading unicode labels was discovered so GParted was switched back to FS specific read_label method first. - older versions of blkid support do not support reading all volume labels. That brings us to where we are today. As you point out, vol_id is no longer available. Hence we could consider removing the file system specific read_label command from hfs and hfsplus file systems. Unfortunately I cannot think of a hfs/hfs+ specific command that can perform the read. If we were to replace vol_id with blkid, then we should use the FS_Info method instead to reduce accessing the disk to retrieve volumen labels that we already have cached in FS_Info.
Hi Sinlu, Another approach to this challenge would be to consider adding a support_label flag, similar to read_label and write_label. Curtis
Hi Curtis, > If we were to replace vol_id with blkid, then we should use the FS_Info method > instead to reduce accessing the disk to retrieve volumen labels that we already > have cached in FS_Info. Good point. > Another approach to this challenge would be to consider adding a support_label > flag, similar to read_label and write_label. read_label currently fulfills two purposes: 1 save whether the fs specific method can be used 2 save whether the filesystem supports labels and the label can be read out by gparted They are in conflict when 1 says "no usage of fs specific method", and 2 says "can be read out". Current implementation strictly follows 1, my first patch proposed to extend fs specific method so that 1 and 2 are equal. Best would be to assign those purposes to separate flags, as you propose. Which purpose should support_label fulfill?
(In reply to comment #6) > Best would be to assign those purposes to separate flags, as you propose. Which > purpose should support_label fulfill? My thoughts are that support_label would mean that the file system supports having a volume label. The flag read_label would mean (1) whether a file system specific method exists to read the label.
Hi Curtis, thank you for your thoughts on this topic. > My thoughts are that support_label would mean that the file system supports > having a volume label. OK. Then support_label is, as it doesn't refer to a capability of GParted itself, but to a capability of the filesystem, not of type Support, but of type bool? https://git.gnome.org/browse/gparted/tree/include/Utils.h?id=GPARTED_0_16_1#n114 We assume with your definition that if GParted can re-format a partition, it can always set the label while reformatting, if the filesystem supports labels.
> OK. Then support_label is, as it doesn't refer to a capability of GParted > itself, but to a capability of the filesystem, not of type Support, but of type > bool? > > https://git.gnome.org/browse/gparted/tree/include/Utils.h?id=GPARTED_0_16_1#n114 Thank you Sinlu for pointing out the code reference where the capabilities of the file system are defined. From looking at the enum Support, a flag of support_label does not fit with the existing enum. I believe that is why you suggested type bool. If I understand correctly, you need a way to determine if a file system can write a label while formatting, but does not have tools to support setting the label on an existing file system. I wonder if there is another way to approach this? If we wanted to keep with the enum Support, we could consider a flag such as: Support create_with_label; //Can and how to format file system with label Then this value would be NONE for file systems that we cannot format with a label (or that we cannot format at all). The value would be usually be EXTERNAL for file systems that we can format with a label. This would more closely match the current enum Support, and should still provide the capability that you seek. What do you think of this approach? Do you have a different suggestion?
Created attachment 246118 [details] [review] second proposal, add create_with_label flag Hi Curtis, I've written a patch to add your proposed create_with_label flag to GParted. > If I understand correctly, you need a way to determine if a file system can > write a label while formatting, but does not have tools to support setting the > label on an existing file system. You do. Actually the flag 'support_label' would be sufficient, as there is already the flag 'create'. &&-ing them together means making the assumption I named in comment 8. In Partition properties dialog this &&-ing would be done through the sensitivity of the filesystem combobox: Only filesystems with 'create' flag are sensitive, and can therefore be selected. Only for selected filesystems the logic decides whether to set the label entry sensitive. But I like the create_with_label idea more, as its not just of the same type as all other flags in struct FS, it also comes out without the assumption, and therefore its more flexible: Imagine having only access to a formating utility for a filesystem with label support, where the utility doesn't support labels on reformat, or its label support is buggy. Then create would be set, and support_label too as theoretically labels are possible or with more advanced utilities labels even can be set on reformat. But of course labels can't be set on reformat in that particular situation. This conflict does not arise when using create_with_label. In such case, create_with_label would be set to NONE. Therefore I think its a good idea to have the create_with_label flag.
Hi Sinlu, The patch in comment #10 looks good to me.Regards, I suggest we move this patch to the patch set in bug 691681 where I believe this patch will be used. When this has been done, we can close this report as NOTABUG because it is desired to keep the vol_id read_label functionality for hfs/+, and because GParted already falls back blkid. Does that sound reasonable? Curtis
Hi Curtis, Thank you for reviewing my patch. Of course, this bug is closely related with bug 691681, but I think that the changes from the patch can be used by new features of GParted even without the patchset from bug 691681. For example, after having added a format operation, the sensitivity of the Partition -> Label dialog can be set depending on the create_with_label flag. The label typed in can then be directly added to the format operation. We could also think of splitting of the second commit of the patchset which merges label operations with format operations. This patch affects lots of files. With every single file affected the probability of a needed rebase rises when new commits come to git master. Therefore I think that adding the patch to git directly, without adding it to the patchset, is possible, too. You're right about that we don't want to change the vol_id read_label functionality anymore, but after we have discovered the root cause, this bug can still remain open, only with another summary, like: "Add create_with_label flag to FS struct"
Change title from: hfs and hfsplus use only vol_id to read label, should fallback to blkid to: Add create_with_label flag to FS struct
The patch in comment #10 has been committed to the git repository. The relevant git commit can be viewed at the following link: Add create_with_label flag to struct FS (#701569) https://git.gnome.org/browse/gparted/commit/?id=20006e1f8ec8fa23e67e7199004632fbeba10afe
The patch to address this report has been included in GParted 0.16.2 released on September 18, 2013.