[blivet:rhel7/master] Set device.format.label field close to where we read it (#1056139)

Vratislav Podzimek vpodzime at redhat.com
Thu Jan 23 05:38:41 UTC 2014


On Wed, 2014-01-22 at 09:58 -0500, Anne Mulhern wrote:
> 
> 
> 
> ----- Original Message -----
> > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> > Sent: Wednesday, January 22, 2014 3:31:32 AM
> > Subject: Re: [blivet:rhel7/master] Set device.format.label field close to	where we read it (#1056139)
> > 
> > On Tue, 2014-01-21 at 12:07 -0500, mulhern wrote:
> > > Resolves: rhbz#1056139
> > > 
> > > Previously, somewhere between where this was set and where it was first
> > > read
> > > in _populate_right_side it was occasionally being reset to None. This
> > > should have been impossible as the code between is very simple and it
> > > should
> > > be impossible for it to have this side-effect. Nonetheless, it was observed
> > > to happen.
> > > 
> > > Signed-off-by: mulhern <amulhern at redhat.com>
> > > ---
> > >  pyanaconda/ui/gui/spokes/custom.py | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/pyanaconda/ui/gui/spokes/custom.py
> > > b/pyanaconda/ui/gui/spokes/custom.py
> > > index b6bbf7c..82e020f 100644
> > > --- a/pyanaconda/ui/gui/spokes/custom.py
> > > +++ b/pyanaconda/ui/gui/spokes/custom.py
> > > @@ -1693,9 +1693,6 @@ class CustomPartitioningSpoke(NormalSpoke,
> > > StorageChecker):
> > >          else:
> > >              use_dev = device
> > >  
> > > -        if hasattr(device.format, "label") and device.format.label is
> > > None:
> > > -            device.format.label = ""
> > > -
> > >          if hasattr(use_dev, "req_disks") and not use_dev.exists:
> > >              self._device_disks = use_dev.req_disks[:]
> > >          else:
> > > @@ -1734,7 +1731,12 @@ class CustomPartitioningSpoke(NormalSpoke,
> > > StorageChecker):
> > >          self._mountPointEntry.set_text(getattr(device.format,
> > >          "mountpoint", "") or "")
> > >          fancy_set_sensitive(self._mountPointEntry,
> > >          device.format.mountable)
> > >  
> > > -        self._labelEntry.set_text(getattr(device.format, "label", ""))
> > > +        if hasattr(device.format, "label"):
> > > +            if device.format.label is None:
> > > +                device.format.label = ""
> > > +            self._labelEntry.set_text(device.format.label)
> > > +        else:
> > > +            self._labelEntry.set_text("")
> > >          fancy_set_sensitive(self._labelEntry, not device.format.exists)
> > Looks good to me, thanks! Just out of curiosity -- if device.format
> > doesn't have the 'label' attribute, does it make sense to just set
> > labelEntry to "" or should it be made insensitive?
> > 
> > --
> > Vratislav Podzimek
> > 
> > Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
> > 
> > _______________________________________________
> > anaconda-patches mailing list
> > anaconda-patches at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> > 
> 
> There is so much that is going on with interactions between labels and formats that it seems better to make the _labelEntry
> sensitivity dependent only on
> a property that will hold true for all formats and set it only in one place.
> Then cope with the user's final choice when they press the button below all those fields.
> 
> Otherwise, with previous implementation, they get into situations where they feel they need to set the filesystem type to one they don't want in order to reset the label
> to what it was originally so that it won't be changed, then change it back to the filesystem type they actually want. Little do they
> know that actually the label will be ignored in this case, so if they hadn't already saved their previous changes, they are OK.
> And there are lots of other, even more complicated issues, like this one.
> 
> I thought that existence was a property that wouldn't change if the user changed the format, but I now realize that that is almost certainly false.
> 
> I believe that it would be very possible for the disk to be already formatted for one filesystem and then if the user changes the format, exists == True would change to exists == False.
> 
> So, I'm going to be working on that problem, separately in association with the original labeling bug, #1038590, but push this fix.
Sounds good to me, thanks!

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list