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 667278 - Add support for setting UUID
Add support for setting UUID
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: 2012-01-04 18:07 UTC by Rogier
Modified: 2012-03-02 12:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for UUID support (57.05 KB, patch)
2012-01-04 18:07 UTC, Rogier
none Details | Review
Patch for UUID support (v2) (54.91 KB, patch)
2012-01-05 21:46 UTC, Rogier
none Details | Review
Patch for UUID support (v3) (57.48 KB, patch)
2012-01-08 13:46 UTC, Rogier
none Details | Review
Patch for UUID support (v4) (57.21 KB, patch)
2012-01-12 17:26 UTC, Rogier
none Details | Review
Patch for UUID support (incremental v3-v4) (1.36 KB, patch)
2012-01-12 17:28 UTC, Rogier
none Details | Review
Patch for UUID support (v4, relative to ee9f6f3) (57.26 KB, patch)
2012-01-13 08:16 UTC, Rogier
none Details | Review
Set UUID support for NTFS (v0.1, relative to main patch v4) (15.69 KB, patch)
2012-01-13 17:09 UTC, Rogier
none Details | Review
Patch for UUID support (v5, relative to ee9f6f3) (57.22 KB, patch)
2012-01-17 18:47 UTC, Rogier
none Details | Review
Set UUID support for NTFS (v1, relative to main patch v5) (15.49 KB, patch)
2012-01-17 18:59 UTC, Rogier
none Details | Review
Set UUID support for exfat (v1, relative to main patch v5) (1.68 KB, patch)
2012-01-17 19:14 UTC, Rogier
none Details | Review
Set UUID support for NTFS (v2, patch 1/4, relative to c814b25) (5.93 KB, patch)
2012-01-25 17:38 UTC, Rogier
none Details | Review
Set UUID support for NTFS (v2, patch 2/4, relative to c814b25) (1.40 KB, patch)
2012-01-25 17:39 UTC, Rogier
none Details | Review
Set UUID support for NTFS (v2, patch 3/4, relative to c814b25) (6.55 KB, patch)
2012-01-25 17:41 UTC, Rogier
none Details | Review
Set UUID support for NTFS (v2, patch 4/4, relative to c814b25) (21.63 KB, patch)
2012-01-25 17:43 UTC, Rogier
none Details | Review
Change UUID patch set in mbox format. (57.78 KB, patch)
2012-01-30 23:03 UTC, Curtis Gedak
none Details | Review
Change UUID patch set 2 in mbox format. (57.94 KB, patch)
2012-02-03 23:33 UTC, Curtis Gedak
none Details | Review
GParted manual changes relative to 'Change UUID patch set 2' by Curtis (3.91 KB, patch)
2012-02-07 15:36 UTC, Rogier
none Details | Review
Fix for set UUID support - remove temp files for fat* (1.05 KB, patch)
2012-02-08 08:48 UTC, Rogier
none Details | Review
Fixes for 'unknown' filesystem problems (3.10 KB, patch)
2012-02-18 16:33 UTC, Rogier
none Details | Review

Description Rogier 2012-01-04 18:07:19 UTC
Created attachment 204594 [details] [review]
Patch for UUID support

GParted needs support for setting UUIDs on filesystems.

I have created a patch for this feature.

Regards,

Rogier
Comment 1 Mike Fleetwood 2012-01-04 23:22:08 UTC
A couple of questions from a novice at GParted code reviewing:
1) Should there be some autoconf check for use of new uuid_* functions?
2) Should the UUID reading functions specific to fat16, fat32, jfs &
   ntfs file systems be left un-implemented, letting the existing
   FS_Info class provide the UUID, as they just use the same blkid
   command?

Mike
Comment 2 Rogier 2012-01-05 08:51:38 UTC
1) I'm not exactly an autoconf expert, so I can't give a definitive answer, but there is no compile-time configuration involved, just like for labeling support, which is conceptually equivalent. I am also not aware of any platform dependencies, but of course I may have overlooked them.

2) I totally agree. I think the file-system specific code should not use generic tools if gparted provides suitable fallback behavior (probably using the same tools anyway). Nevertheless, I decided to implement it for consistency: several filesystems can't read labels, but still have an implementation of the read_label method, using another generic tool: vol_id. In my opinion, either both should be in, or both should be out.

I didn't use vol_id in my code, because debian does not provide it (It was removed about 2.5 years ago). As far as I can tell, it seems that vol_id has been deprecated / removed in favor of blkid.

Rogier
Comment 3 Curtis Gedak 2012-01-05 18:09:38 UTC
First I would like to start by thanking Rogier for creating this bug report and attaching a patch to add the ability to write UUIDs.  I am confident that such a feature will be welcomed by GParted users.  I also appreciate your time and consideration of critiques by others.

Secondly thank you Mike for your code review.  We can all learn from others codes and I appreciate each persons insight.


Following are my responses to the code review questions:

1)  Since the availability or absence of the file system commands are determined at run-time, there is no need for any autoconf configuration code.
In fact, if GParted is running and a user installs a needed package, then a click of the "Rescan for Supported Actions" in the File System Support dialog will recognize the added commands.

Summary:  No patch re-work is required regarding this question.


2)  If a file system specific tool is available, then it is best to write a read_uuid implementation in the file system class.

Since using blkid (used by FS_Info class) is the fall back if a volume label or now a UUID is empty, the use of this command should be avoided for FAT16 and FAT32.
See GParted_Core::set_device_partitions( Device & device ).
Because the fall back FS_Info class caches results, the fall back method does not require an extra disk access and should be faster.

Rogier, I agree that there is no need to add vol_id.  The use of vol_id in other file system classes is due to legacy code.

Summary:  Please remove the read_uuid implementation for FAT16 and FAT32.  That is unless you find a file system specific implementation. 


*New* 3)  Please remove trailing whitespace from patch.
Even though the GParted code is riddled with trailing spaces and tabs, I would like to work towards slowly cleaning this up.  Hence with new patches I do not want to make the problem worse.

When I applied the patch from the initial post, I received many warnings regarding trailing whitespace.  If you have the time, could you remove these trailing whitespace characters from the patch?

Summary:  Please remove trailing whitespace characters from patch.


Currently I am working on reviewing several patches, bug reports, and forum questions, so your patience is much appreciated.  :-)
Comment 4 Mike Fleetwood 2012-01-05 20:58:03 UTC
Looked closer at GParted's configure(.in) script, and it already checks
for libuuid using uuid_generate().  (GParted doesn't currently call any
APIs but it looks like it is needed by libparted dependency).  Nothing
more needed here.
Comment 5 Rogier 2012-01-05 21:46:25 UTC
Created attachment 204710 [details] [review]
Patch for UUID support (v2)
Comment 6 Rogier 2012-01-05 21:48:42 UTC
Hi Mike & Curtis,

Thank you for your feedback. I particularly appreciate the speed of the process !

> Summary:  Please remove the read_uuid implementation for FAT16 and FAT32.  That
> is unless you find a file system specific implementation. 

I haven't found a file-system specific implementation, so I've just removed blkid from the file system specific code in my patch (FAT16/FAT32/NTFS/JFS).

> Summary:  Please remove trailing whitespace characters from patch.

grep '^+.*[ ^I]$' gives no output now.

Kind Regards,

Rogier.
Comment 7 Rogier 2012-01-08 13:46:06 UTC
Created attachment 204833 [details] [review]
Patch for UUID support (v3)

I have discovered how to obtain the uuid from fat filesystems (using the not-immediately-obvious 'mdir'), as well as from jfs. See the updated patch.

If you'd like an incremental patch instead, please let me know.

Rogier.
Comment 8 Curtis Gedak 2012-01-08 16:56:53 UTC
Thanks Rogier for continuing to work on this.  The all-inclusive patch you included will be fine.
Comment 9 Curtis Gedak 2012-01-10 18:13:37 UTC
Hi Rogier, I am working through various tests on your patch and much of it is working quite well.  In general the setting of "New UUID" works as I would expect.  :-)

While testing and reviewing the code I have discovered a few opportunities for enhancement.

1)  The set UUID patch changes the label behaviour.

With patches it is preferred to limit the patch changes to the bug or feature being addressed.  In this case the patch is to add the new feature set UUID.  As such the patch should not change the label behaviour.

The code I am referring to is in src/Win_GParted.cc:

-               //only allow labelling of real partitions that support labelling
-               if ( selected_partition .status == GParted::STAT_REAL && fs .writ
+               //allow labelling of all partitions that support labelling
+               if ( fs .write_label )
                        allow_label_partition( true ) ;

This change introduces a bug into GParted labelling of new partitions.  To see the bug for yourself use the following steps:

   a)  Create a new partition (but do not apply the operation)
   b)  Set the label on this new partition
   c)  Apply these two operations.

The set label operation will fail because the partition name is unknown
(E.g., New Partition #1).


2)  The "New UUID" operation fails on new partition operations.

This is a similar problem to the bug introduced in point 1.  Essentially, the "New UUID" operation should only be available for real, already existing partitions.


3)  There is an unneeded #ifdef clause in GParted_Core::change_uuid.

The code in question is the following few lines:

+#ifndef HAVE_LIBPARTED_3_0_0_PLUS
+                       case FS::LIBPARTED:
+                               break ;
+#endif

I suspect that this code was copied from another section.  Prior to libparted version 3.0, there was some file system specific support in libparted.  With libparted 3.0 and higher, this file system support has been removed.  If libparted did support reading or writing UUIDs, then it might be useful to include the clause.  However, as far as I know libparted does not support reading or writing UUIDs.


In conclusion, the above three items are the opportunities for improvement that I have found so far.  If you have any questions on these, or other points, then please do ask.  We are all learning here and I make mistakes too.  ;-)
Comment 10 Curtis Gedak 2012-01-10 19:57:34 UTC
Today I discovered how to set the UUID of an NTFS file system partition.

The command to set the UUID of an NTFS partition is:

   dd if=/dev/urandom of=/dev/[ntfspartition] bs=8 count=1 seek=9

Alternatively. you could swap the block size and count values and use:

   dd if=/dev/urandom of=/dev/[ntfspartition] bs=1 count=8 seek=72

Where:  /dev/[ntfspartition] is something like /dev/sda1


Following is the link where I discovered this information (see post #10):

   Changing the UUID of an NTFS Partition
   http://ubuntuforums.org/showthread.php?t=1240146


For more detailed information on the NTFS Boot Record, see the following link:

   NTFS Boot Record Secrets
   http://thestarman.pcministry.com/asm/mbr/NTFSBR.htm
Comment 11 Rogier 2012-01-11 13:15:08 UTC
Hi Curtis, 

Thanks for the feedback.

All three points in your first comment are obvious, even without checking the code, now that you told me about them :-)

Wrt to point 1 (and hence 2): Actually, I did already notice the behavior, but I didn't link it to a change I made. 
My apologies for not being more careful about it.

Wrt your second comment, I assumed you'd only want to use existing tools...

I also found the following thread about UUIDs on NTFS:

     Feature request: Utility to fix geometry info & change UUID
     http://tuxera.com/forum/viewtopic.php?f=2&t=21525&hilit=uuid

It provides some valuable additional information.
Instead of using the dd command (actually, according to the thread, at least *two* dd commands would be needed...), I propose that I followup on the thread. Until it is supported natively, I propose to create a small external program instead. This keeps the gparted code clean of what I'd consider a hack, and it may be of use to other people.

I'll revise the patch. Please let me know if you object to my proposal of adding a temporary 'ntfsuuid' tool to gparted.

Rogier.
Comment 12 Curtis Gedak 2012-01-11 17:21:12 UTC
Thank you for your quick response Rogier.

(In reply to comment #11)
> Wrt to point 1 (and hence 2): Actually, I did already notice the behavior, but
> I didn't link it to a change I made. 
> My apologies for not being more careful about it.

No worries.  I have introduced a few bugs myself.  Not on purpose of course, but these things do happen.  :)

 
> Wrt your second comment, I assumed you'd only want to use existing tools...

I do consider "dd" to be an existing tool.  It might not be file system specific, but is a part of many GNU/Linux distributions.  "dd" has been used in the past with GParted to copy partitions, and also to set the hidden sectors in an NTFS file system when it was moved/copied.

For example see the following commit:

Remove requirement for xxd and dd for NTFS move or paste action
http://git.gnome.org/browse/gparted/commit/?id=dd8a57a8fec3abfa8b1b5399a73aa6a5845aed23

 
> I also found the following thread about UUIDs on NTFS:
> 
>      Feature request: Utility to fix geometry info & change UUID
>      http://tuxera.com/forum/viewtopic.php?f=2&t=21525&hilit=uuid
> 
> It provides some valuable additional information.
> Instead of using the dd command (actually, according to the thread, at least
> *two* dd commands would be needed...), I propose that I followup on the 
> thread.

My preference would be to simply use "dd" for now.  Only one call to dd is required since we do not care if the results from /dev/urandom are not byte reversed.  All that is needed is a new unique value.

If you meant that a second call to "dd" is needed to change the backup sector, then yes, that would be the 100% correct way to perform the update.

As it stands now, changes to the NTFS file system set the dirty flag, so when a user reboots into Windows, a file system check (chkdsk) is run.  This ensures the integrity of the file system.  Hence a single change to the primary NTFS boot record should be sufficient for our purposes.

Of course if you would like to make a second call to "dd" to update the backup sector, then I am certainly on-board with such a decision.


> Until it is supported natively, I propose to create a small external program
> instead. This keeps the gparted code clean of what I'd consider a hack, and it
> may be of use to other people.
> 
> I'll revise the patch.

Thank you Rogier.  I do appreciate your consideration of these changes, and your efforts to improve the patch.


> Please let me know if you object to my proposal of
> adding a temporary 'ntfsuuid' tool to gparted.

My preference is to not create a temporary "ntfsuuid" tool.
My reasons are as follows:

A)  The 'dd' solution is simple, easy to implement, and easy to maintain.
B)  The proper location for an 'ntfsuuid' tool is with the ntfs-3g (previously
    ntfsprogs) project/package.
C)  The creation of a temporary 'ntfsuuid' tool will require more effort and
    maintenance during it's lifetime.
D)  It would be preferable to spend the time for creating a temporary
    'ntfsuuid' tool directly in the ntfs-3g project.  That way the
    development effort is more productively spent on a tool that will
    be packaged with the NTFS file system tools.
E)  We can change from the 'dd' solution to the one provided in ntfs-3g when
    it is available.
F)  Creating separate file system tools is outside of the GParted Mission
    Statement that we use to guide development.

If you have questions, or comments, please feel free to express these.
Comment 13 Rogier 2012-01-12 17:12:35 UTC
Hi Curtis,

Thank you for your response, and for your effort in helping me getting this patch right.

> > Wrt your second comment, I assumed you'd only want to use existing tools...
> 
> I do consider "dd" to be an existing tool.  It might not be file system
> specific, but is a part of many GNU/Linux distributions.  "dd" has been used in
> the past with GParted to copy partitions, and also to set the hidden sectors in
> an NTFS file system when it was moved/copied.
> 
> For example see the following commit:
> 
> Remove requirement for xxd and dd for NTFS move or paste action
> http://git.gnome.org/browse/gparted/commit/?id=dd8a57a8fec3abfa8b1b5399a73aa6a5845aed23

I don't quite agree with you on this. Please consider the following reasoning:

IMHO, there is a fundamental difference between these two uses. dd was meant to move/copy bytes, not to edit filesystems. It seems to me that a program like gparted that uses external programs to implement part of its functions should, as a matter of principle, use such external programs only for purposes that they can be expected to handle correctly.

NTFS tools are created for manipulating ntfs filesystems. They can be depended upon to do their due diligence, and to fail if they detect some inconsistency. When using dd to edit UUIDs, which is not more than opening the device and writing to it, GParted pokes around in on-disk-data, which means that:
1) GParted is responsible for doing this only under the right circumstances, and making sure it does not inadvertently damage the filesystem.
2) The maintenance burden, to ensure that the correct bytes are modified in the correct way, lies with GParted.

Wrt to point 1 above: In my impression, the design and implementation of GParted are not geared towards being careful about exactly which bytes of a filesystem are being modified. It, rightfully, depends on the external tools it uses to detect and fail when confronted with inconsistencies and unexpected circumstances.

Wrt to point 2 above: As long as ntfs-3g (or some other tool) does not support changing the UUID, it follows that GParted has the choice between not supporting it, and bearing the maintenance burden associated with poking around in a filesystem.

Of course, maintaining a simple invocation of 'dd' is easier to maintain than a separate program, but a simple invocation of 'dd' (or two invocations, for that matter) is not sufficient, and a separate program is more self-contained, and therefore easier to oversee & maintain, without having to take into account the possible influence of other parts of GParted. (However unlikely that seems in this case).

In order to illustrate my point, I made up the following test case. It may be contrived, but the scenario is certainly not so improbable as to be impossible:
- Create a block device, and format it to NTFS.
- Start GParted, and wait until filesystem detection has completed
- Reformat the device to something else. Preferably FAT16 or FAT32, as they write significant data to the first sector :-)
- Use GParted to change the UUID on the device
- Apply

I ran this testcase after implementing support for changing the NTFS UUID using 'dd'.
Result: GParted writes 8 random bytes to the first sector of a non-ntfs filesystem. With two 'dd' commands, it would also have written random data to the very last sector of the filesystem, which is much more likely to be harmful.

In order to ensure that this, or similar things, won't happen, and in order to be complete, the implementation would have to do the following:
- Verify the disk device has not been mounted since the previous check
- Verify the disk device is indeed (still) an NTFS filesystem
- Generate a new UUID & update the first sector
- Verify that the backup sector is present (i.e. valid), and if so, update it.
- Signal the OS to rescan the device (to make it aware of the new UUID).

To me, it seems that this amount of complexity is better not integrated into GParted, but is better handled in a stand-alone tool.

Obviously, this functionality should be available as part of ntfs-3g, and any other solution is less preferable, and should be replaced with an ntfs-3g based implementation as soon as one is available.

I have posted a message to the Tuxera forums to ask about the status of UUID (or rather 'volume serial number' - which it is in NTFS reality) changing support in NTFS-3G.

> > Please let me know if you object to my proposal of
> > adding a temporary 'ntfsuuid' tool to gparted.
> 
> My preference is to not create a temporary "ntfsuuid" tool.
> My reasons are as follows:
> 
> A)  The 'dd' solution is simple, easy to implement, and easy to maintain.
> B)  The proper location for an 'ntfsuuid' tool is with the ntfs-3g (previously
>     ntfsprogs) project/package.
> C)  The creation of a temporary 'ntfsuuid' tool will require more effort and
>     maintenance during it's lifetime.
> D)  It would be preferable to spend the time for creating a temporary
>     'ntfsuuid' tool directly in the ntfs-3g project.  That way the
>     development effort is more productively spent on a tool that will
>     be packaged with the NTFS file system tools.
> E)  We can change from the 'dd' solution to the one provided in ntfs-3g when
>     it is available.
> F)  Creating separate file system tools is outside of the GParted Mission
>     Statement that we use to guide development.
> 
> If you have questions, or comments, please feel free to express these.

In summary, I think:
- Using dd is risky, as dd is inconsiderate.
- Using dd does also come with a maintenance burden
- Using dd alone is not enough: more complex processing is needed
- The functionality should indeed, and preferably be in ntfs-3g

In conclusion, and given the result of my test, I don't feel comfortable creating a GParted patch that uses 'dd' to edit a filesystem, and putting my name on it.

For the moment, I'll be awaiting the (final) result of my posting the the Tuxera forum.

Kind Regards,

Rogier.
Comment 14 Rogier 2012-01-12 17:26:47 UTC
Created attachment 205118 [details] [review]
Patch for UUID support (v4)

Hereby a revised patch incorporating fixes for the 3 issues.

UUID support for NTFS not (yet?) included. See previous post.
Comment 15 Rogier 2012-01-12 17:28:25 UTC
Created attachment 205119 [details] [review]
Patch for UUID support (incremental v3-v4)

And an incremental version
Comment 16 Curtis Gedak 2012-01-12 18:13:23 UTC
Thank you Rogier for your considered and detailed response, and for the updated patches.

I think we are mostly on the same page in that we agree that support for changing the NTFS UUID should lie with the NTFS file system tools (currently ntfs-3g).


(In reply to comment #13)
> Wrt to point 2 above: As long as ntfs-3g (or some other tool) does not support
> changing the UUID, it follows that GParted has the choice between not
> supporting it, and bearing the maintenance burden associated with poking
> around in a filesystem.
> 
> Of course, maintaining a simple invocation of 'dd' is easier to maintain than
> a separate program, but a simple invocation of 'dd' (or two invocations, for
> that matter) is not sufficient, and a separate program is more
> self-contained, and therefore easier to oversee & maintain, without having to
> take into account the
> possible influence of other parts of GParted. (However unlikely that seems in
> this case).
> 
> In order to illustrate my point, I made up the following test case. It may be
> contrived, but the scenario is certainly not so improbable as to be
> impossible:
> - Create a block device, and format it to NTFS.
> - Start GParted, and wait until filesystem detection has completed
> - Reformat the device to something else. Preferably FAT16 or FAT32, as they
> write significant data to the first sector :-)
> - Use GParted to change the UUID on the device
> - Apply
> 
> I ran this testcase after implementing support for changing the NTFS UUID
> using 'dd'.
> Result: GParted writes 8 random bytes to the first sector of a non-ntfs
> filesystem. With two 'dd' commands, it would also have written random data to
> the very last sector of the filesystem, which is much more likely to be
> harmful.

In the above steps I assume that you reformatted the partition to another file system from outside GParted.  And I agree that this list of steps, can certainly occur.

Unfortunately I do not know how to prevent such outside behaviour so this problem is very real for many operations within GParted.

Even forcing a complete device refresh before applying the operations does not guarantee that a partition will not be somehow changed outside of GParted while the operation is being applied.

My thoughts are that at some point the user has to take responsibility for his/her actions.  We cannot guarantee that a user will not abuse a tool set.  I think the best we can do is to try to guide the user so as to minimize the chance of harm occurring.


> In order to ensure that this, or similar things, won't happen, and in order to
> be complete, the implementation would have to do the following:
> - Verify the disk device has not been mounted since the previous check
> - Verify the disk device is indeed (still) an NTFS filesystem
> - Generate a new UUID & update the first sector
> - Verify that the backup sector is present (i.e. valid), and if so, update it.
> - Signal the OS to rescan the device (to make it aware of the new UUID).
> 
> To me, it seems that this amount of complexity is better not integrated into
> GParted, but is better handled in a stand-alone tool.

I wholeheartedly agree that this level of complexity belongs in a dedicated file system tool set.

 
> Obviously, this functionality should be available as part of ntfs-3g, and any
> other solution is less preferable, and should be replaced with an ntfs-3g
> based implementation as soon as one is available.
> 
> I have posted a message to the Tuxera forums to ask about the status of UUID
> (or rather 'volume serial number' - which it is in NTFS reality) changing
> support in NTFS-3G.

Thank you for following up in the Tuxera forums.  A solution here would be the best for all.  :-)

If there is no solution with ntfs-3g in the near future, I might create a 'dd' type patch myself.  My reasoning being that using GParted to set the NTFS UUID would, in my opinion, be safer than having a user type in the dd command directly.


> In summary, I think:
> - Using dd is risky, as dd is inconsiderate.
> - Using dd does also come with a maintenance burden
> - Using dd alone is not enough: more complex processing is needed
> - The functionality should indeed, and preferably be in ntfs-3g

These are good points, especially the complexity required to try to prevent incorrectly writing to the wrong file system.

 
> In conclusion, and given the result of my test, I don't feel comfortable
> creating a GParted patch that uses 'dd' to edit a filesystem, and putting my
> name on it.

I understand and respect your position on this set ntfs uuid issue.

Thank you again for all your work on this issue.  Your perspective and efforts are appreciated.  :)

Sincerely,
Curtis Gedak
Comment 17 Curtis Gedak 2012-01-12 20:56:11 UTC
Rogier, would you be able to rebase your "Patch for UUID support (v4)" against the latest git repository?

Since I have committed changes to the master branch that affect btrfs and nilfs2, the current patch does not apply cleanly.

If you are busy with other things then no worries.  I will look into doing this probably mid next week.
Comment 18 Rogier 2012-01-13 08:16:28 UTC
Created attachment 205165 [details] [review]
Patch for UUID support (v4, relative to ee9f6f3)

Hi Curtis,

I merged the git repository master branch into my branch. I verified the diff and in particular the changes to btrfs.cc and nilfs2.cc. The merge was painless, and, as far as I can see, everything is OK.

The new patch is relative to commit ee9f6f3.

(For the record: the previous patches were all relative to commit 7477ba3.)

Rogier.
Comment 19 Rogier 2012-01-13 13:05:09 UTC
Hi anybody,

A patch to ntfs-3g for changing the UUID (or actually, the volume serial number) on NTFS filesystems has been created by Tuxera for testing & feedback. Does anybody like to have a look at it besides me ? Of particular interest would be somebody owning an MS Windows system for test purposes (expect to reinstall from scratch :-), as I don't own any.

BTW: tinkering with the serial number may affect the windows product activation, so I have read.

Please let me know.

Rogier.
Comment 20 Rogier 2012-01-13 17:09:56 UTC
Created attachment 205213 [details] [review]
Set UUID support for NTFS (v0.1, relative to main patch v4)

This is an additional patch which enables setting UUIDs on NTFS.

This patch requires a patched version of ntfslabel to actually be able to change the UUID. It is not final yet.

Rogier.
Comment 21 Curtis Gedak 2012-01-13 21:57:21 UTC
Thank you Rogier for the rebased patch, and also the separate patch for NTFS set UUID.

My testing with the patch in comment #18 is going well.  Since I have a few other things to attend to, it will likely be mid next week before I finish a second review of the code and begin checking the documentation additions.

In terms of committing this code, which name and userid would you like used?
If you would prefer to create a patch with a commit message then we can do that after we finish reviewing/testing/updating the code.

Regarding the NTFS set UUID support, you might wish to try posting a note in the GParted testers forum asking if others might help with testing.  In the past I have not had much luck looking for testers, but you just never know.
http://gparted.org/forum.php
Comment 22 Rogier 2012-01-17 18:47:28 UTC
Created attachment 205473 [details] [review]
Patch for UUID support (v5, relative to ee9f6f3)

Hi Curtis,

I have made some more changes. The main patch just has a cosmetic change - the added file in POTFILES.in and Makefile.am are now in their alphabetically correct position.

Rogier.
Comment 23 Rogier 2012-01-17 18:59:38 UTC
Created attachment 205474 [details] [review]
Set UUID support for NTFS (v1, relative to main patch v5)

Hi Curtis,

Hereby the new NTFS patch, which depends on the to-be-released version of ntfs-3g. It has only been tested with a patched version of the current release.

I also added 'ntfs-3g' as alternative to the list of tools required for NTFS.

I would like to have your feedback in particular on two aspects:
- I used a string constant containing a localized string to identify how much of the UUID is changed (UUID_RANDOM and UUID_RANDOM_NTFS_HALF). I am not sure whether that will work in general though. It does for me :-)
- I added quite a bit of explanation in the change-UUID dialog box, to warn Windows users. My impression is, it is a bit too much, but I couldn't think of a better alternative. What's your opinion ?

Rogier
Comment 24 Rogier 2012-01-17 19:14:09 UTC
Created attachment 205475 [details] [review]
Set UUID support for exfat (v1, relative to main patch v5)

Hereby an addtional patch with support for reading UUIDs for exfat. (last week, I found exfat-utils in debian testing). That makes it about the first thing that works on exfat :-)

I also added the 'exfat-utils' to the list of tools needed.

Rogier
Comment 25 Rogier 2012-01-18 11:48:38 UTC
(In reply to comment #21)
> In terms of committing this code, which name and userid would you like used?

I guess the name will be what shows up as author in git ? You can copy my full name from the license info at the top of the added files.

What is the userid used for ? I assume 'Rogier' will do as far as I am concerned.

> If you would prefer to create a patch with a commit message then we can do that
> after we finish reviewing/testing/updating the code.

I haven't developed any preference. Whatever works best for you will be OK.

Rogier.
Comment 26 Curtis Gedak 2012-01-19 21:07:52 UTC
Thank you Rogier for the updates in patch 5 in comment #22.  This latest patch looks good and my testing so far has been successful.

I have reviewed this patch an I am ready to commit it to the git master branch with a few changes:

1)  I plan to remove the DEBUG code in GParted_Core::set_device_partitions() for the UUID extra marker to keep the code simpler.

2)  I plan to remove the ifndefs section for HAVE_LIBPARTED_3_0_0_PLUS in GParted_Core::read_uuid()


If you agree with the above, I will perform the commit.

In comment #21 I goofed.  I meant to ask which name and email you would like used in the commit so that you get the proper credit for this update.

E.g., 
Author: Rogier Goossens <goossens.rogier@gmail.com>

Is the above E.g. correct?


(In reply to comment #23)
> I would like to have your feedback in particular on two aspects:
> - I used a string constant containing a localized string to identify how much
> of the UUID is changed (UUID_RANDOM and UUID_RANDOM_NTFS_HALF). I am not sure
> whether that will work in general though. It does for me :-)

I still need to look closer at this patch.

My initial thought is that if there are potential Windows Activations problems when changing the whole UUID, why don't we just change only HALF?

This would still make the complete UUID unique and hence avoid duplicate UUID problems.  Also, the code can be simplified immensely so that a special case for NTFS will not be required.

What do you think?

> - I added quite a bit of explanation in the change-UUID dialog box, to warn
> Windows users. My impression is, it is a bit too much, but I couldn't think of
> a better alternative. What's your opinion ?

If you agree with changing only HALF of the UUID for NTFS, then we can probably remove much of the confusion that we introduce by offering users the ability to change either the whole or half of an NTFS UUID.


(In reply to comment #24)
> Hereby an addtional patch with support for reading UUIDs for exfat. (last
> week, I found exfat-utils in debian testing). That makes it about the first
> thing that works on exfat :-)
> 
> I also added the 'exfat-utils' to the list of tools needed.

Thanks for this extra patch.  There are some potential licensing concerns with exfat so I am taking a delayed approach with this file system.  If the big distributions (e.g., Fedora, Ubuntu, SUSE) pick up exfat-utils, then would be the time to reconsider this patch.  As you might have already seen, there is an outstanding patch in Bug #639760 - exfat / fat64 support.
Comment 27 Rogier 2012-01-21 12:15:05 UTC
(In reply to comment #26)
> 1)  I plan to remove the DEBUG code in GParted_Core::set_device_partitions()
> for the UUID extra marker to keep the code simpler.
>
> 2)  I plan to remove the ifndefs section for HAVE_LIBPARTED_3_0_0_PLUS in
> GParted_Core::read_uuid()

No problem of course. For the debug code, I already half expected you not to want it in...

> My initial thought is that if there are potential Windows Activations problems
> when changing the whole UUID, why don't we just change only HALF?

> This would still make the complete UUID unique and hence avoid duplicate UUID
> problems.  Also, the code can be simplified immensely so that a special case
> for NTFS will not be required.

It wouldn't be the most complex piece of NTFS-specific code :-)

> What do you think?

I have been thinking so far, that a choice for half or full would actually belong in an 'expert' mode, rather than being offered to everybody. For casual users, GParted should indeed change only half. 

However, I also think that most probably, FAT suffers from the same problem, except that it has no more than 'half' a serial number...  In addition, if only half the serial number is (silently) changed for NTFS, questions and bug reports are bound to arise, from users 'complaining' that only half of the UUID was changed...

Also, even though there is a generic warning about the risks, and the need to backup, I think in this case, a specific warning should be issued as well, about the possible side-effects of this particular operation.  Without such a warning, after editing partitions using GParted, the user could be unpleasantly suprised by the fact that it caused Windows to need reactivation. There is also no easy way to reverse the process. So, even if the user *did* save the old serial number (which, AFAIK, doesn't happen during a regular backup), (s)he'd still need an expert to get it back in place. The only solution would therefore be to reactivate. In some cases, if the user has already reactivated recently before, that may not even be that easy.

So, I would lik to propose that I remove the choice between half and full, but still keep a dialog with a warning text and 'OK' and 'Cancel' buttons. The dialog would apply to NTFS, but also to FAT filesystems.

Would that be OK with you ?

Rogier.
Comment 28 Curtis Gedak 2012-01-23 19:38:42 UTC
The patch (5) in comment #22, with some minor documentation changes and code changes listed above, has been committed to the Gnome git repository for inclusion in the next release of GParted.

The relevant git commit can be viewed at the following link:
http://git.gnome.org/browse/gparted/commit/?id=9e96159bb2e6a843ac9465b340ad5f32b0320937

Thank you again Rogier for this patch.  I plan to respond comment #27 soon.
Comment 29 Curtis Gedak 2012-01-23 21:21:09 UTC
(In reply to comment #27)
> So, I would lik to propose that I remove the choice between half and full, but
> still keep a dialog with a warning text and 'OK' and 'Cancel' buttons. The
> dialog would apply to NTFS, but also to FAT filesystems.
> 
> Would that be OK with you ?

This sounds like a reasonable approach.  It is important to let the user know that there might be Windows Activation problems.  If I recall, Windows XP could be installed on FAT or NTFS file systems.
Comment 30 Curtis Gedak 2012-01-23 23:00:21 UTC
Hi Rogier,

I've been thinking about your proposal in comment #27.  Ideally we should try to implement the UUID dialog in a way that is generic.  The file system specific stuff should ideally be stored with each file system.

If the UUID dialog displays a message with 'OK' and 'Cancel' buttons, we might be able to make the dialog generic, yet have the text message come from the file system specific files (e.g., ntfs.cc, fat16.cc, fat32.cc).  This would make the solution much more extensible for future operating systems.

If there is no text message, then perhaps the dialog box would not need to be displayed.

Anyways this is just a thought.  You might have a better idea on how to implement this in a generic way.
Comment 31 Rogier 2012-01-24 16:20:32 UTC
(In reply to comment #30)
Hi Curtis,

> I've been thinking about your proposal in comment #27.  Ideally we should try
> to implement the UUID dialog in a way that is generic.  The file system
> specific stuff should ideally be stored with each file system.

Absolutely.

> If the UUID dialog displays a message with 'OK' and 'Cancel' buttons, we might
> be able to make the dialog generic, yet have the text message come from the
> file system specific files (e.g., ntfs.cc, fat16.cc, fat32.cc).  This would
> make the solution much more extensible for future operating systems.

> Anyways this is just a thought.  You might have a better idea on how to
> implement this in a generic way.

I do have a few thoughts, but I basically don't know what the needs of other filesystems might be in the future. It's difficult to abstract when there really is only one use-case...

Adding an 'UUID_message' to every filesystem would be too specific & clutter the code of most filesystems who don't need it, and probably never will.

Adding a way for filesystems to be able to tune dialog boxes (which would also allow file-system specific mkfs options for instance) seems nice, but is quite a lot of work. I would consider that a separate project.

Adding a generic method to get custom text strings from a filesystem, which could be used for other purposes might be an option. One other use would be the 'mount/umount' text for instance. For swap, it would return 'swapon/swapoff' instead, for lvm it might be 'export/import', etc.

What do you think ?

One complication I am running into, is that I see no easy way to get from a partition to a FileSystem object in a dialog box. Ideally, partitions would contain a pointer to a filesystem object. Adding one seems another separate project though. I might do that, but not for this patch...
Am I overlooking something ? Or is this indeed not straightforward ?

Regards,

Rogier.
Comment 32 Curtis Gedak 2012-01-24 17:47:21 UTC
(In reply to comment #31)
> (In reply to comment #30)
> > If the UUID dialog displays a message with 'OK' and 'Cancel' buttons, we might
> > be able to make the dialog generic, yet have the text message come from the
> > file system specific files (e.g., ntfs.cc, fat16.cc, fat32.cc).  This would
> > make the solution much more extensible for future operating systems.
> 
> > Anyways this is just a thought.  You might have a better idea on how to
> > implement this in a generic way.
> 
> I do have a few thoughts, but I basically don't know what the needs of other
> filesystems might be in the future. It's difficult to abstract when there
> really is only one use-case...
> 
> Adding an 'UUID_message' to every filesystem would be too specific & clutter
> the code of most filesystems who don't need it, and probably never will.
> 
> Adding a way for filesystems to be able to tune dialog boxes (which would also
> allow file-system specific mkfs options for instance) seems nice, but is quite
> a lot of work. I would consider that a separate project.
> 
> Adding a generic method to get custom text strings from a filesystem, which
> could be used for other purposes might be an option. One other use would be the
> 'mount/umount' text for instance. For swap, it would return 'swapon/swapoff'
> instead, for lvm it might be 'export/import', etc.
> 
> What do you think ?

This last option you list has some potential. As you point out though there is only the one use case for UUID and NTFS so perhaps this is not yet the time to delve into this additional complexity.

> One complication I am running into, is that I see no easy way to get from a
> partition to a FileSystem object in a dialog box. Ideally, partitions would
> contain a pointer to a filesystem object. Adding one seems another separate
> project though. I might do that, but not for this patch...
> Am I overlooking something ? Or is this indeed not straightforward ?

I believe that you understand this correctly.  When the original author created GParted, the Partition object was set up to represent not just a partition, but also the file system and unallocated space.  Refactoring this for a dialog box seems like overkill for this purpose.


Let us try taking an overall view of the problem again with respect to NTFS.

In normal use, there is no need to change the UUID of a partition.  When the partition is created the UUID is unique and as such no strange mounting problems should occur.

The problem with duplicate UUIDs occurs when the partition is copied and then used on the same computer in conjunction with the source partition.  As I see it, this is the problem we are trying to address.

Changing the full UUID on copied file systems solves the duplicate UUID problem.  The challenge in the case of the Windows boot partition (C:) is that activation will likely be required if the user plans to use the copied partition as a boot partition.

I believe that it is sufficient for us to warn the user of this potential problem.  There might be further licensing concerns if we try to resolve the problem directly by only changing half of the UUID.  We would not want to invite legal trouble for our users.

With this in mind, a simple dialog box similar to that used when moving partitions might be a good solution for now.

For example see Win_GParted::activate_resize():
http://git.gnome.org/browse/gparted/tree/src/Win_GParted.cc#n1535

What are your thoughts on this?

Do you think it is important to have 'OK' and 'Cancel' options on the dialog box?
Comment 33 Rogier 2012-01-24 19:24:29 UTC
Hi Curtis

(In reply to comment #32)
> I believe that it is sufficient for us to warn the user of this potential
> problem.  There might be further licensing concerns if we try to resolve the
> problem directly by only changing half of the UUID.  We would not want to
> invite legal trouble for our users.
> 
> With this in mind, a simple dialog box similar to that used when moving
> partitions might be a good solution for now.

> What are your thoughts on this?

I agree.
I think that we see things in basically the same way.

As the mount/umount and swapon/swapoff text in the menu is already an issue, and the LVM patch will add another pair, and as FAT and NTFS require different warning texts, I think I would favor adding the custom text function now. It's also a minor change anyway. 
Either a new function to map a FILESYSTEM value to a FileSystem object, or a modified GParted_Core to make its function accessible and usable for the purpose would then solve the FileSystem object problem for the moment.

> Do you think it is important to have 'OK' and 'Cancel' options on the dialog
> box?

Actually, not really. I hadn't thought of the fact that the operation is not applied anyway, giving the user ample opportunity to undo.

Rogier.
Comment 34 Curtis Gedak 2012-01-24 19:34:09 UTC
(In reply to comment #33)
> As the mount/umount and swapon/swapoff text in the menu is already an issue,
> and the LVM patch will add another pair, and as FAT and NTFS require different
> warning texts, I think I would favor adding the custom text function now. It's
> also a minor change anyway. 
> Either a new function to map a FILESYSTEM value to a FileSystem object, or a
> modified GParted_Core to make its function accessible and usable for the
> purpose would then solve the FileSystem object problem for the moment.

You have convinced me that now is the time to handle the custom text function.  Please feel free to implement the function as you best see fit with an eye towards future maintainability.
Comment 35 Rogier 2012-01-25 17:38:19 UTC
Created attachment 206115 [details] [review]
Set UUID support for NTFS (v2, patch 1/4, relative to c814b25)

Hi Curtis,

Hereby the new set-UUID patch for NTFS.
As not only NTFS-related code was changed, I split the patch into 4.

This is: Patch 1/4 for Set UUID support for NTFS.

Short description:
Make FileSystem objects in GParted_Core accessible and usable by others

Regards,

Rogier
Comment 36 Rogier 2012-01-25 17:39:45 UTC
Created attachment 206116 [details] [review]
Set UUID support for NTFS (v2, patch 2/4, relative to c814b25)

Patch 2/4 for Set UUID support for NTFS.

Short description:
Add support for custom text strings depending on the filesystem
Comment 37 Rogier 2012-01-25 17:41:57 UTC
Created attachment 206117 [details] [review]
Set UUID support for NTFS (v2, patch 3/4, relative to c814b25)

Patch 3/4 for Set UUID support for NTFS.

Short description:
Use custom text functions for mount/unmount and swapon/swapoff texts
Comment 38 Rogier 2012-01-25 17:43:46 UTC
Created attachment 206118 [details] [review]
Set UUID support for NTFS (v2, patch 4/4, relative to c814b25)

Patch 4/4 for Set UUID support for NTFS.

Short description:
Implement changing UUID for NTFS
Comment 39 Curtis Gedak 2012-01-27 19:00:16 UTC
Hi Rogier.  Thank you for breaking these changes into four separate diff files.  I am working my way through this set of 4 diffs.

So far I have no questions on the first two diffs.

With the third diff I have the following comments/answers to questions:


a)  I do not think that we need to mark the "--placeholder--" for translation in Win_GParted.cc.  The reason being that this string should be replaced before the menu is displayed.  Hence the "--placeholder--" text string should never be shown to the user.


b)  Regarding the following Win_GParted.cc comment you are correct:
    //TODO: IIRC, some filesystems can only be resized when mounted (??) ...
File systems such as btrfs can only be resized when mounted.

However, GParted does not currently support this because the partition sizes cannot be changed and the kernel informed of the change when the file system is mounted.

Hence for now, GParted requires the file system to be unmounted for any partition change actions.  GParted will perform the mount and unmount as required to perform the file system resize.  The partition will be changed when the file system is unmounted.

There is some enhancement work going on in this area.  Phillip Susi has some kernel patches outstanding to permit changing a partition size while the file system is mounted.  However to my knowledge these changes have not been incorporated into the Linux kernel yet.

When this ability is available, then some refactoring of the GParted code will be required to directly support online resizing and _partition_ resizing.


I haven't looked closely at the fourth diff yet.
Comment 40 Rogier 2012-01-27 21:12:58 UTC
(In reply to comment #39)

> a)  I do not think that we need to mark the "--placeholder--" for translation
> in Win_GParted.cc.  The reason being that this string should be replaced before
> the menu is displayed.  Hence the "--placeholder--" text string should never be
> shown to the user.

Obvious... :-)

> b)  Regarding the following Win_GParted.cc comment you are correct:
>     //TODO: IIRC, some filesystems can only be resized when mounted (??) ...
> File systems such as btrfs can only be resized when mounted.
> 
> However, GParted does not currently support this because the partition sizes
> cannot be changed and the kernel informed of the change when the file system is
> mounted.

Actually, LVM logical volumes already can...

> When this ability is available, then some refactoring of the GParted code will
> be required to directly support online resizing and _partition_ resizing.

I have realized that...
I think that as soon as LVM logical volume support is available in GParted, LVM users will want to resize their logical volumes and their filesystems (insofar as they support it) while they are in use... It is one of the selling points of LVM... (though certainly not the most important selling point).

Obviously, not everything can be done at once. LVM support by itself, even without online resizing, is already a big win (as far as my judgement goes. I don't know how widespread its use actually is - I, for one, couldn't live without it.).

Regards,

Rogier.
Comment 41 Curtis Gedak 2012-01-27 22:49:16 UTC
(In reply to comment #40)
> (In reply to comment #39)
> > However, GParted does not currently support this because the partition sizes
> > cannot be changed and the kernel informed of the change when the file system > > is mounted.
> 
> Actually, LVM logical volumes already can...

'Sorry for any confusion.  I was thinking more of LVM Physical Volumes when I made the above statement (comment #39).  As you state, LVM LVs can be resized on-line.  :-)

I think the plan for initial LVM support in GParted should be to support LVM Physical Volume resizing.  Mainly because this is not easy to do for LVM users as it requires two steps:  partition resizing and pvresizing.

Resizing LVM LVs, on the otherhand, is relatively simple since the command line tools perform this function quite handily.

Since resizing LVM PVs will require changing the partition size we are again caught by the inability of the Linux kernel to change partition sizes on-the-fly so to speak.
Comment 42 Curtis Gedak 2012-01-28 18:19:37 UTC
I am in the process of reviewing the fourth patch for setting the NTFS UUID.  Your implementation of the custom text feature is novel and from my testing is working quite well.

Thank you again Rogier for updating the GParted help documentation.  This area is often overlooked.  Here the most important thing here is to capture the content, and you have done just that.

When writing documentation that will be translated, there are several guidelines that can make the end result more understandable and more easily translatable to other languages.  Following is a link that I use to try to make the help documentation align with Gnome philosophies:

GNOME Documentation Style Guide V1.6
http://developer.gnome.org/gdp-style-guide/stable/


The important task of capturing the content is done.  The secondary task of word-smithing the content to align with the Gnome Documentation Style Guide remains.

Would you like to tackle the word-smithing challenge?

If not then I will gladly do this on your behalf (i.e., you get credit for the patch including documentation).
Comment 43 Rogier 2012-01-29 16:16:19 UTC
Hi Curtis,

I have had a look at the Gnome Documentation Style Guide, and although I would gladly do the word-smithing and improve understandability, translatability, etc., I am not familiar with (all of) the Gnome rules, so I am a bit daunted by the task of finding exactly what would need rewording and why. In addition, even if I did try to find the violations, I suspect that I'd overlook many of them, and maybe introduce new ones, not significantly easing the task in the end.

So, the best option may be for you to do the word-smithing, and then for me to observe the result, and try to learn from it.

Rogier.
Comment 44 Curtis Gedak 2012-01-29 17:41:02 UTC
Quite understandable.  I will word-smith the text and then post the updates back here for your review.  When we agree on the changes I can then commit the changes to the Gnome git repository.
Comment 45 Curtis Gedak 2012-01-30 23:03:00 UTC
Created attachment 206481 [details] [review]
Change UUID patch set in mbox format.

Hi Rogier,

This is a heads up that because I have made some major changes while
word-smithing the text to align to the Gnome Documentation Style
Guide.  As such I have created a separate commit for these changes.

The attached file includes your four patches (with minor formatting
changes), and a fifth patch for re-working the translatable text.

You can apply this patch set to today's git repostory with the following command:

     git am change-uuid-patchset.mbox

Since my changes were fairly significant, I thought it best to capture
these in a separate commit.  That way the history of your inclusions
is not lost, especially in case we need to re-add some of the
knowledge you originally captured in the translatable strings.


Following is an attempt to explain my reasoning for the changes:

1) As a general rule, I try to follow the KISS philosophy.  That is
"Keep It Simple, Stupid!".  With documentation my interpretation of
this philosophy is to state only what is relevant, in as few words as
possible, and only in the appropriate section.


2) Removed comment about UUIDs not being a panacea from caution in the
"Deleting a Partition" section.

Reason: The problem with duplicate UUIDs belongs in the "Copying and
Pasting a Partition" section, because this is where the problem
arises.


3) The sections for "UUIDs in Linux" and "UUIDs in Windows" have been
removed, and their essential content incorporated into other existing
sections.

Reasons:  As I see it the the key concepts I extracted and tried to shorten
          are:

Under "Copying and Pasting a Partition":

a) Duplicate UUIDs can cause problems with mounting, and might lead to
severe data corruption or loss.

Under "Changing a Partition UUID":

b) A new UUID on a system C: partition might invalidate the Windows
Production Activation key.

c) In an attempt to avoid invalidating the WPA, on NTFS file systems
only half of the UUID is set to a new random value.

d) If the WPA is invalidated you will need to re-activate Windows to login.

e) For FAT and NTFS file systems, the Volume Serial Number is treated
as the UUID.

f) The WPA should not be affected by changing the UUID of data partitions
or removable media partitions.

g) A new UUID can cause GNU/Linux to fail to boot or mount a file
system.  This can be fixed by editing the files involved in mounting,
such as /etc/fstab.


Please let me know what you think of these changes (good or bad).
Comment 46 Curtis Gedak 2012-02-03 17:33:07 UTC
Hi Rogier,

I was just re-reading my last comment and was worried that it could be misinterpreted.

Specifically when I mentioned the KISS philosophy, I meant no harm.  Some background explanation of KISS can be found on Wikipedia:
http://en.wikipedia.org/wiki/KISS_principle

My intention was not to offend in any way.

Sincerely,
Curtis
Comment 47 Curtis Gedak 2012-02-03 23:33:35 UTC
Created attachment 206744 [details] [review]
Change UUID patch set 2 in mbox format.

Attached is an updated change UUID patch set in mbox format that has been rebased against today's git repository (d2d830108b4f8fb1876430ad745e7788f4ae9dd9).

Please note that this patch set does not include an enhancement for the custom text function for LVM2 PVs.

Rogier, when you have a chance I would like to know what you think of this patch set.


Please note that we can edit previous commits (those not yet committed to the master branch).  We do this by making note of the previous commit number and then issuing the command:

$ git rebase bbc643cd^ --interactive

Where bbc643cd^ is the beginning of the previous commit number.

In the default editor, modify 'pick' to 'edit' in the line whose commit you want to modify. Make your changes and then stage them with

$ git add <filepattern>

Now you can use

$ git commit --amend

to modify the commit, and after that

$ git rebase --continue

to return back to the previous head commit.


The above text is adapted from the following link:
http://stackoverflow.com/questions/1186535/how-to-modify-a-specified-commit
Comment 48 Rogier 2012-02-07 15:36:20 UTC
Created attachment 206991 [details] [review]
GParted manual changes relative to 'Change UUID patch set 2' by Curtis

Hi Curtis,

> Please note that this patch set does not include an enhancement for the custom
> text function for LVM2 PVs.

I assumed the exact text to be used would only be known when LVM write support is finalized ?
Or did I miss a location where custom LVM2 texts should have been added now ?

> Rogier, when you have a chance I would like to know what you think of this
> patch set.

All in all, I think your changes to the manual are an improvement.

In general, I'd like to educate the user, and explain not only that there may be a problem, but also when, when not, and why, to enable the user to make better decisions for himself.

OTOH, I understand that most users will not be interested in the details, and all the information I provide would be too much to grasp anyway.

I would have liked the more detailed information to be available somewhere though, as a service to / background information for the more advanced user.

I do have some suggestions for improvement, which I'm attaching as patch to your version. Note that I removed the paragraph mentioning the windows login issue, as it basically repeats what is said two paragraphs earlier.

Kind Regards,

Rogier.
Comment 49 Rogier 2012-02-08 08:48:34 UTC
Created attachment 207068 [details] [review]
Fix for set UUID support - remove temp files for fat*

Hi Curtis,

I discovered an omission in the UUID code for FAT*: the temporary files are not removed after reading the UUID...

Attached patch fixes the issue.

Regards,

Rogier.
Comment 50 Curtis Gedak 2012-02-08 17:56:47 UTC
Hi Rogier,

(In reply to comment #48)
> > Please note that this patch set does not include an enhancement for the 
> > custom text function for LVM2 PVs.
> 
> I assumed the exact text to be used would only be known when LVM write support
> is finalized ?

You are absolutely correct.  I just wished to make note of this so that when LVM write support is ready we hopefully won't miss this custom text opportunity.

> Or did I miss a location where custom LVM2 texts should have been added now ?

Nope.  :-)


> > Rogier, when you have a chance I would like to know what you think of this
> > patch set.
> 
> All in all, I think your changes to the manual are an improvement.
> 
> In general, I'd like to educate the user, and explain not only that there may
> be a problem, but also when, when not, and why, to enable the user to make
> better decisions for himself.
> 
> OTOH, I understand that most users will not be interested in the details, and
> all the information I provide would be too much to grasp anyway.
> 
> I would have liked the more detailed information to be available somewhere
> though, as a service to / background information for the more advanced user.

It is somewhat of a balancing act when creating on-line help documentation.  This type of documentation is not often read from start to finish.  Also users are often looking for quick answers.

In depth documentation is useful and needed, but I think it does not belong in an on-line help manual.  Having said that, simple short sentences or paragraphs that summarize the in depth knowledge are useful.

We could certainly consider publishing more in depth documentation on the GParted web site.  Also we can include links to such information from the GParted web site to help our users learn more about disk storage concepts.


> I do have some suggestions for improvement, which I'm attaching as patch to
> your version. Note that I removed the paragraph mentioning the windows login
> issue, as it basically repeats what is said two paragraphs earlier.

Thank you for these improvements, which go further to clarify the implications of UUID changes.

I have one question.  In the following sentence is there a word that is more descriptive than "gratuitously" so that translators will choose the correct meaning?

     Gratuitously changing the UUID might cause a GNU/Linux system to
     fail to boot, or to fail to mount a file system.

I looked up the definition of "gratuitous" and I think your intention was to caution users that there are implications to changing the UUID without any forethought.  As we both know, any change to the UUID has implications.

Definition of gratuitous
http://www.merriam-webster.com/dictionary/gratuitous
Comment 51 Rogier 2012-02-08 20:03:12 UTC
(In reply to comment #50)
> (In reply to comment #48)
> We could certainly consider publishing more in depth documentation on the
> GParted web site.  Also we can include links to such information from the
> GParted web site to help our users learn more about disk storage concepts.

I'll think about it.

I'm still unsure as to whether I think it should be on the user's system, or on the GParted site, or both.

Would it be an option at all to put such information somewhere in the GParted distribution, in some form ? I assume that in that case, it would end up on the filesystem of most distributions.

> I have one question.  In the following sentence is there a word that is more
> descriptive than "gratuitously" so that translators will choose the correct
> meaning?
> 
>      Gratuitously changing the UUID might cause a GNU/Linux system to
>      fail to boot, or to fail to mount a file system.
> 
> I looked up the definition of "gratuitous" and I think your intention was to
> caution users that there are implications to changing the UUID without any
> forethought.  As we both know, any change to the UUID has implications.

I may have used the word incorrectly. English is not my native tongue.

And you are correct that even if it was used correctly, it might be subject to being misunderstood and hence mis-translated. I should have thought of that...

'without need / good grounds' would be what I intended to say.

I added it because, while changing the UUID may have undesirable consequences, in some circumstances (in particular, after copying a filesystem), the UUID *should* be changed, and conversely, failure to do so will have undesirable consequences. I don't want users to be confused when in one place the manual recommends to change the UUID, and somewhere else it warns against changing it.

So, something like:

        Changing the UUID when there is no need to do so, may cause a
        GNU/Linux system to fail to boot, or to fail to mount a file system.

might be better.

Kind Regards,

Rogier.
Comment 52 Curtis Gedak 2012-02-08 20:12:32 UTC
(In reply to comment #51)
> So, something like:
> 
>         Changing the UUID when there is no need to do so, may cause a
>         GNU/Linux system to fail to boot, or to fail to mount a file system.
> 
> might be better.

Since the above wording could be misinterpreted to imply that there will be no problem if a user _needs_ to change the UUID, my thoughts are that it might be better to use the following statement:

      Changing the UUID might cause a GNU/Linux system to
      fail to boot, or to fail to mount a file system.

This is because changing the UUID at anytime (with or without cause or need) might cause a GNU/Linux system to fail to boot.

If you agree with the following wording, I will update the patch in comment #48 accordingly and commit it to the repository.  I will then also commit the patch from comment #49 too.
Comment 53 Curtis Gedak 2012-02-08 22:26:40 UTC
(In reply to comment #51)
> (In reply to comment #50)
> > (In reply to comment #48)
> > We could certainly consider publishing more in depth documentation on the
> > GParted web site.  Also we can include links to such information from the
> > GParted web site to help our users learn more about disk storage concepts.
> 
> I'll think about it.
> 
> I'm still unsure as to whether I think it should be on the user's system, or
> on the GParted site, or both.
> 
> Would it be an option at all to put such information somewhere in the GParted
> distribution, in some form ? I assume that in that case, it would end up on 
> the filesystem of most distributions.

Regarding a separate file to be deposited into the user's file system, I do not think that would be very useful.  Mainly because I do not think the user would ever discover it to read it.

I wish I had a hard and fast rule to define what should be included in the help manual, and what should be excluded to keep the manual short and easily read.

One example might be descriptions of file systems.

It would be useful in the help manual to describe in a sentence or two some of the good and bad points of each file system to help a user choose which one to use when creating a new partition.

On the other hand, I think it would be far too much information to outline all the details of the file system, such as the algorithms and b-trees used to allocate files.  This in depth information belongs in a separate document that is probably maintained by each file system project.

Hopefully the above example helps to explain what might seem to be arbitrary decisions on my part.
Comment 54 Rogier 2012-02-09 09:51:26 UTC
(In reply to comment #52)
> (In reply to comment #51)
> > So, something like:
> > 
> >         Changing the UUID when there is no need to do so, may cause a
> >         GNU/Linux system to fail to boot, or to fail to mount a file system.
> > 
> > might be better.
> 
> Since the above wording could be misinterpreted to imply that there will be no
> problem if a user _needs_ to change the UUID,

Isn't that actually the case ?
If the user just copied a partition, there will be no problem if the UUID is changed, but there will most likely be a problem if it is *not* changed....

> my thoughts are that it might be
> better to use the following statement:
> 
>       Changing the UUID might cause a GNU/Linux system to
>       fail to boot, or to fail to mount a file system.
> 
> This is because changing the UUID at anytime (with or without cause or need)
> might cause a GNU/Linux system to fail to boot.

The way I see it, after creating a duplicate partition, changing the UUID of one of the copies poses no other risk than the universal risk associated with editing partitions.

I would even go as far as to suspect that not changing a UUID when needed is far more dangerous than changing it when not needed.

_Changing_ a UUID gives only the risk of not booting, or not being able to mount a filesystem, which is annoying but can be fixed. _Not_changing_ a UUID when needed could cause (I haven't verified it though) severe data corruption and/or loss when partitions are selected at random, in particular when using filesystems that span multiple partitions/disks, like LVM, MD-Raid, btrfs, ...

Maybe the wording I chose does not adequately or unambiguously convey the meaning I intend it to have. However, I do think the manual shoud be clear about the fact that there are different sides to changing UUIDs, and that what may be dangerous and discouraged in some circumstances may actually be necessary, not to say mandatory in others.

I expect that not being clear about it, will confuse users who read the manual - I must admit though, that I haven't asked them about it :-)

Anyway, if you prefer the wording you propose, then that's OK, even if that means it doesn't mention the aspects that I personally think should be mentioned.

Obviously, what is really needed, is that GParted automagically changes the UUID when needed after copying. However, that requires some more changes (like the ability to *not* change the UUID - to make a forensic copy). It is something I am currently working towards. In the end, my goal is that the casual user should not need to worry about UUIDs.

Kind Regards,

Rogier.
Comment 55 Curtis Gedak 2012-02-09 16:50:55 UTC
Thank you Rogier for your patience and perseverance to explain your position.  I think I can now see the concern you are trying to address.

I try to make each individual section of the documentation standalone.  However, there is a potential problem if the user first reads the section on "Copying a Partition.  Then after reading the duplicate UUID warning the user then reads the section "Changing a Partition UUID".  In this section the user would see the warning about "Changing the UUID might cause a GNU/Linux system to fail to boot, or to fail to mount a file system."

This might cause the user to question whether the warning about the duplicate UUIDs, or the warning about changing the UUID breaking boot is the warning to follow.

With this in mind, what do you think about the following warning in the "Changing a Partition UUID" section?

      Changing the UUID to prevent duplicate UUIDs is
      required when both the source and the copy of a
      partition remain on the same computer.  However;
      changing the UUID when there is no need to do so might
      cause a GNU/Linux system to fail to boot, or to fail
      to mount a file system.

My goal is to ensure that the warnings make sense if the user reads any portion or combination of sections within the documentation.
Comment 56 Curtis Gedak 2012-02-09 16:56:43 UTC
Of course after I posted this I realized that these two sentences should be reversed to ensure that the warning is listed first.  ;)

      Changing the UUID when there is no need to do so might
      cause a GNU/Linux system to fail to boot, or to fail
      to mount a file system.  However, changing the UUID to
      prevent duplicate UUIDs is required when both the
      source and the copy of a partition remain on the same
      computer.

Please let me know your thoughts on this version of the warning text.
Comment 57 Rogier 2012-02-10 08:28:49 UTC
(In reply to comment #55)
> Thank you Rogier for your patience and perseverance to explain your position. 
> I think I can now see the concern you are trying to address.
>
> I try to make each individual section of the documentation standalone. 
> However, there is a potential problem if the user first reads the section on
> "Copying a Partition.  Then after reading the duplicate UUID warning the user
> then reads the section "Changing a Partition UUID".  In this section the user
> would see the warning about "Changing the UUID might cause a GNU/Linux system
> to fail to boot, or to fail to mount a file system."
> 
> This might cause the user to question whether the warning about the duplicate
> UUIDs, or the warning about changing the UUID breaking boot is the warning to
> follow.

That is exactly my concern indeed.

Thank you for _your_ patience in understanding my point of view.

(In reply to comment #56)
>       Changing the UUID when there is no need to do so might
>       cause a GNU/Linux system to fail to boot, or to fail
>       to mount a file system.  However, changing the UUID to
>       prevent duplicate UUIDs is required when both the
>       source and the copy of a partition remain on the same
>       computer.
> 
> Please let me know your thoughts on this version of the warning text.

That captures what I intended. I propose a few minor changes:

    Changing the UUID when there is no need to do so might
    cause a GNU/Linux system to fail to boot, or to fail
    to mount a file system.  Changing the UUID is only
    required after copying a partition, to prevent duplicate
    UUIDs when both the source and the copy of the partition
    are used on the same computer.

Kind regards,

Rogier.
Comment 58 Curtis Gedak 2012-02-10 17:46:41 UTC
The patches in comment #35, comment #36, comment #37, comment #38, (all rebased in comment #47), comment #48, and comment #49 have been committed to the Gnome git repository for inclusion in the next release of GParted.

Some minor changes have been made to align with Gnome documentation standards and to try to align the help manual warning text with the application warning dialogs.

The relevant git commits can be viewed at the following links:

Make FileSystem objects in GParted_Core accessible and usable by others
http://git.gnome.org/browse/gparted/commit/?id=ce9feeda0e9a04da04cec0a1b01512ed68c2495c

Add support for custom text strings depending on the filesystem
http://git.gnome.org/browse/gparted/commit/?id=8735227dd7def467dad315097cb783f99bef2056

Use custom text functions for mount/unmount and swapon/swapoff texts
http://git.gnome.org/browse/gparted/commit/?id=170a79b3a08cbcd20415ea02d12a3995539ed9e8

Implement changing UUID for NTFS (#667278)
http://git.gnome.org/browse/gparted/commit/?id=4108daf15f2bb6eecb54bd98a9175620fe6e8030

Restructure and word-smith UUID translatable text
http://git.gnome.org/browse/gparted/commit/?id=2f191307cfda60c1a4ebb9b15a3aed794c15b68d

Further improve help manual wording
http://git.gnome.org/browse/gparted/commit/?id=a5eb91e549109a8533ac0f2a6f827b2e2c651ec0

Remove temporary file after reading UUID of fat16 and fat32 filesystems
http://git.gnome.org/browse/gparted/commit/?id=88cfe40a8b1ce59ab8e1ed56ac029e998547d51b
Comment 59 Curtis Gedak 2012-02-15 18:28:20 UTC
Rogier, I just discovered an anomaly that I think is caused by the series of patches in this bug report.

The anomaly occurs when you are viewing the dialog to create a new partition.  There is a new file system type called "unknown" that is at the top of the list.  This "unknown" entry was not shown in GParted 0.11.0.

Do you have time to look at this anomaly and create a patch?

If not then I can investigate further.
Comment 60 Rogier 2012-02-18 16:33:29 UTC
Created attachment 207936 [details] [review]
Fixes for 'unknown' filesystem problems

Hi Curtis,

The problem is an off-by-one error in Dialog_Partition_New.cc, which I fixed in the first attached patch. The bug existed, but the change from commit http://git.gnome.org/browse/gparted/commit?id=ce9feeda0e9a04da04cec0a1b01512ed68c2495c caused it to surface.

The second patch fixes a potential problem that could be caused by the fact that the 'unknown' filesystem is no longer (necessarily) last in the FILESYSTEMS array. This bug was actually introduced by the commit mentioned above.

Rogier.
Comment 61 Rogier 2012-02-18 17:01:03 UTC
BTW, what is the procedure for updates to the website ?

Rogier
Comment 62 Curtis Gedak 2012-02-18 17:50:18 UTC
Thank you Roger for investigating and fixing this anomaly.  I like your solution because it should be more robust since it does not depend on file system positioning.

The two patches in comment #60 have been committed to the repository for inclusion with the GParted 0.12.0 release.

To keep the commit log text less than 80 characters wide, I reworked the commit log messages.  This is a best practice for git that I read somewhere that suggests wrapping text at 72 characters.  These shorter line widths help with the display of git logs in standard size terminal windows (80 characters).

The relevant git commits can be viewed at the following links:

Fix for implicit assumption that 'FS_UNKNOWN' is last in FILESYSTEMS list
http://git.gnome.org/browse/gparted/commit/?id=387feb143a8623ce056458c591c5d0f1ee5d8116

New partition dialog: first filesystem in list is always included
http://git.gnome.org/browse/gparted/commit/?id=1197a521741316868922b3e8b6db24059b481d63
Comment 63 Curtis Gedak 2012-02-18 20:59:55 UTC
In my testing I have discovered another regression.

Unfortunately in a previous code review, I missed seeing that the setting of the write label capability for linux-swap was accidentally overwritten.

The following patch to restore write label capability has been committed to the repository for inclusion in GParted 0.12.0:

Restore write label capability for linux-swap
http://git.gnome.org/browse/gparted/commit/?id=1c0ea2783db849f3bbfb76ad5d83b3577c0b1d64
Comment 64 Curtis Gedak 2012-02-22 00:49:20 UTC
This enhancement has been included in GParted 0.12.0 released on February, 21, 2012.
Comment 65 Markus Elfring 2012-03-02 12:33:40 UTC
I would like to comment the addition of the interface "get_custom_text(..., int index = 0)".
http://git.gnome.org/browse/gparted/commit/?id=4108daf15f2bb6eecb54bd98a9175620fe6e8030

How do you think about to work with unsigned integers there when the corresponding value will be used as an array index?