On Fri, 2012-10-26 at 11:02 -0700, Brian C. Lane wrote:
On Fri, Oct 26, 2012 at 10:45:34AM +0200, Vratislav Podzimek wrote:
> On Thu, 2012-10-25 at 13:04 -0700, Brian C. Lane wrote:
> > From: "Brian C. Lane" <bcl(a)redhat.com>
> >
> > You need at least one of the platform's disklabels in order for the free
> > space on the disk to be useful. If none of the disks labels will work we
> > set the free space to 0 and allow the resize dialog to be used to remove
> > all the partitions which will relabel the disk.
> > ---
> > pyanaconda/ui/gui/spokes/storage.py | 20 ++++++++++++++++----
> > 1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/pyanaconda/ui/gui/spokes/storage.py
b/pyanaconda/ui/gui/spokes/storage.py
> > index 14ea404..7aab762 100644
> > --- a/pyanaconda/ui/gui/spokes/storage.py
> > +++ b/pyanaconda/ui/gui/spokes/storage.py
> > @@ -644,10 +644,22 @@ class StorageSpoke(NormalSpoke, StorageChecker):
> > disks = [d for d in self.disks if d.name in self.selected_disks]
> > disks_size = sum(Size(spec="%f MB" % d.size) for d in
disks)
> >
> > - free_space = self.storage.getFreeSpace(disks=disks,
> > -
clearPartType=CLEARPART_TYPE_NONE)
> > - disk_free = sum([f[0] for f in free_space.itervalues()])
> > - fs_free = sum([f[1] for f in free_space.itervalues()])
> > + # Figure out if the existing disk labels will work on this platform
> > + # you need to have at least one of the platform's labels in order
for
> > + # any of the free space to be useful.
> > + disk_labels = set([disk.format.labelType for disk in self.disks \
> > + if hasattr(disk.format,
"labelType")])
> > + platform_labels = set(self.storage.platform.diskLabelTypes)
> > + if disk_labels and platform_labels.isdisjoint(disk_labels):
> > + disk_free = 0
> > + fs_free = 0
> > + log.debug("Need disklabel: %s have: %s" % (",
".join(platform_labels),
> > + ",
".join(disk_labels)))
> > + else:
> > + free_space = self.storage.getFreeSpace(disks=disks,
> > +
clearPartType=CLEARPART_TYPE_NONE)
> > + disk_free = sum([f[0] for f in free_space.itervalues()])
> > + fs_free = sum([f[1] for f in free_space.itervalues()])
> Shouldn't this change go directly to storage.getFreeSpace? If nothing
> else text mode (in future) will make a use of it too I think.
I've re-examined this, including text mode this time :) Text mode
actually works mostly ok, since the user is deciding which method to
use. The only problem there was that it didn't show the storage errors
and I've sent a patch for that.
Yeah, that's why I added "(in
future)". If text mode is meant to be
full-featured in future, we should make these pieces of code available
for it. But I understand that we should fix GUI in first place.
We can't move this down into getFreeSpace for a couple reasons:
1. It is used in other places (the usage summary) where, I think, we
still want to know the total amount of space available.
2. We don't want to zero out the space in all the disks, we want to
invalidate their use if there is no valid disklabel. Doing this inside
getFreeSpace would mean kludging in something to set the disk values to
zero. It is cleaner to do it here, which is the only place it is needed.
Both sound
reasonable, thanks for the explanation.
I also spotted a problem with the above code, it should be examining
disks, not self.disks and I've fixed and tested that in my working
branch.
+ disk_labels = set([disk.format.labelType for disk in disks \
+ if hasattr(disk.format,
"labelType")])
Good catch. And summed up -- ACK.
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic