On Fri, 2013-07-19 at 17:53 -0700, Brian C. Lane wrote:
On Fri, Jul 19, 2013 at 10:40:07AM +0200, Vratislav Podzimek wrote:
> On e.g. virtual machines with smalls disks and high amount of
> memory it doesn't make sense to create large swap space leaving
> only a little for the system and user data.
>
> Also change a comment to a docstring and document parameters.
>
> Resolves: rhbz#878907
> Related: rhbz#879232
> if self.recommended or self.hibernation:
> - (self.size, self.maxSizeMB) =
iutil.swapSuggestion(hibernation=self.hibernation)
> + (self.size, self.maxSizeMB) = \
> + iutil.swapSuggestion(hibernation=self.hibernation,
disk_space=disk_space)
Breaking like that makes it less readable. I don't think we really need
to stick with a strict 80 column max anymore. But if you do want to
break the line, do it after the first argument and align the others
with the first:
(self.size, self.maxSizeMB) = iutil.swapSuggestion(hibernation=self.hibernation, \
disk_space=disk_space)
Yeah,
that looks better and the backslash is not needed that way. Fixing locally.
> diff --git a/storage/partitioning.py b/storage/partitioning.py
> index e20a18f..16b5823 100644
> --- a/storage/partitioning.py
> +++ b/storage/partitioning.py
> @@ -244,6 +244,20 @@ def doAutoPartition(anaconda):
> # sanity check the individual devices
> log.warning("not sanity checking devices because I don't know how
yet")
>
> + # get a new swap suggestion, now that we know the total disk space we're
> + # gonna use
> + disk_space = 0
> + for disk in anaconda.id.storage.disks:
> + if not anaconda.id.storage.clearPartDisks \
> + or disk.name in anaconda.id.storage.clearPartDisks:
> + disk_space += disk.size
> +
> + (min_size, max_size) = iutil.swapSuggestion(disk_space=disk_space)
> + for request in anaconda.id.storage.autoPartitionRequests:
> + if request.fstype == "swap":
> + request.size = min_size
> + request.maxSize= max_size
> +
I think this needs to be inside a check for
anaconda.id.storage.doAutoPart so that it doesn't clobber swap
partition sizes specified by kickstarts.
You are right, I thought
'doAutoparition' is not called in case of
kickstart autopartitioning, but obviously it is. Good catch! Fixing
locally as well.
I've tested the updated version of this patch by going through as much
code paths as possible and everything seems to work as expected.
Anything else I should change or shall I push it?
Thanks,
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic