Updated: [rhinstaller/anaconda/pulls/33 f22-branch] master/f22-branch - Set the default fs label when reformatting (#1141981)
If the new format has a default label, and a label hasn't been set by the user, use the default. This allows the ESP to be reformatted and used without removing it and recreating it.
From: "Brian C. Lane" bcl@redhat.com
On Mac we want the FS label to be 'Linux HFS+ ESP' so we can detect previous installations. In custom the user may not know this, so force the FS label to be the correct one when it is selected. --- pyanaconda/ui/gui/spokes/custom.py | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py index a278842..1cc6883 100644 --- a/pyanaconda/ui/gui/spokes/custom.py +++ b/pyanaconda/ui/gui/spokes/custom.py @@ -977,6 +977,11 @@ def _save_right_side(self, selector): new_device_info["encrypted"] = encrypted
# FS LABEL + # The Linux HFS+ ESP partition needs to have the correct FS label. + # Override anything the user sets for it. + if fs_type == "macefi": + self._labelEntry.set_text(new_fs.label) + label = self._labelEntry.get_text() old_label = getattr(device.format, "label", "") changed_label = (label != old_label)
I think this change this shouldn be happening in _populate_right_size instead of _save_right_side.
No, I only want to make the change after the user has selected the filesystem. If it happens in populate it won't be forced to the right label when update it hit. populate picks up the change when it is made, so everything looks right.
How about?
``` diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py index 625cf0a..c76f40f 100644 --- a/pyanaconda/ui/gui/spokes/custom.py +++ b/pyanaconda/ui/gui/spokes/custom.py @@ -2405,6 +2405,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker): return log.debug("fs type changed: %s", new_type) fmt = getFormat(new_type) + if fmt.label: + self._labelEntry.set_text(fmt.label) fancy_set_sensitive(self._mountPointEntry, fmt.mountable)
def _populate_container(self, device=None): ```
The previous attempts at this changed other behavior, really we just want to make sure that the filesystem label is correct on macefi.
The problem with the previous patch is that label, whether it is None or "" or "user string" gets passed all the way down into blivet and it ends up not actually setting a label at all, which isn't what we want.
I took a look at the blivet commit 45b1e921 and bug and I'm now quite uncertain that the constructor added in that call is doing the right thing for blivet. I mean, if I were using blivet w/out anaconda, and I did ```
from blivet import formats z = formats.getFormat("macefi")
``` is it really right that the label should be:
```
z.label
'Linux HFS+ ESP' ```
In every other case, not specifying the label means accept the default label that mkfs will set. In this one case, it means set the label to "Linux HFS+ ESP". Is that the behavior that any client of blivet ought to expect? If not, it would be best to get rid of that constructor in blivet, and just go with a slightly extended patch in anaconda, that sets the format label, like:
``` diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py index 625cf0a..dd53bfa 100644 --- a/pyanaconda/ui/gui/spokes/custom.py +++ b/pyanaconda/ui/gui/spokes/custom.py @@ -2404,7 +2404,9 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker): if new_type is None: return log.debug("fs type changed: %s", new_type) - fmt = getFormat(new_type) + fmt = getFormat(new_type, label="Linux HFS+ ESP" if new_type == "macefi" else None) + if fmt.label: + self._labelEntry.set_text(fmt.label) fancy_set_sensitive(self._mountPointEntry, fmt.mountable)
def _populate_container(self, device=None): ```
macefi is a very special case that I created so that on Mac hardware we could easily create a HFS+ formatted ESP and detect it for reuse. So, yes, setting the label is correct, that's the whole reason it exists.
I understand that setting the label is a necessary behavior in anaconda. My question is, is it the correct behavior for blivet? That is, is easily creating a HFS+ formatted ESP and detecting it for reuse a blivet problem, or really just an anaconda one?
Blivet is where most of the logic for this lives so yes, I'd say that's where it should stay.
Ok.
Then I think:
``` diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py index 625cf0a..c76f40f 100644 --- a/pyanaconda/ui/gui/spokes/custom.py +++ b/pyanaconda/ui/gui/spokes/custom.py @@ -2405,6 +2405,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker): return log.debug("fs type changed: %s", new_type) fmt = getFormat(new_type) + if fmt.label: + self._labelEntry.set_text(fmt.label) fancy_set_sensitive(self._mountPointEntry, fmt.mountable)
def _populate_container(self, device=None): ```
must be the correct way to go.
Oh, that's in on_fs_type_changed, I was thinking it was in populate right side. The problem with putting it there is that it will get set, but if they select something else the label will stick, the user won't notice and we'll end up with a mis-labeled ext4 partition.
Doing it in save_right_side does it at the last moment, when we know for sure that's the fstype they actually want to use.
What about?
``` [mulhern@localhost anaconda]$ git diff diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py index 625cf0a..1d9558c 100644 --- a/pyanaconda/ui/gui/spokes/custom.py +++ b/pyanaconda/ui/gui/spokes/custom.py @@ -2405,6 +2405,7 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker): return log.debug("fs type changed: %s", new_type) fmt = getFormat(new_type) + self._labelEntry.set_text(fmt.label or "") fancy_set_sensitive(self._mountPointEntry, fmt.mountable)
def _populate_container(self, device=None): ```
No, then it'll always clear whatever the user sets -- normally the label's going to be something the user wants to set for a mountpoint. I want to make this as limited as possible.
``` diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py index 625cf0a..8310a65 100644 --- a/pyanaconda/ui/gui/spokes/custom.py +++ b/pyanaconda/ui/gui/spokes/custom.py @@ -2405,6 +2405,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker): return log.debug("fs type changed: %s", new_type) fmt = getFormat(new_type) + + self._labelEntry.set_text(fmt.label or self._labelEntry.get_text()) fancy_set_sensitive(self._mountPointEntry, fmt.mountable)
def _populate_container(self, device=None): ```
?
I feel like we're going around in circles here. I'm standing by my last commit.
I feel inhibited about acking that commit because I don't think it's right. But I do think this is taking up too much time. I'm happy for someone else to ack it, if they see fit.
It seems like the label entry should be populated and disabled for fs types that have a required label.
Closed.
anaconda-patches@lists.fedorahosted.org
-
dashea