Re: [PATCH 2/4] Break the complex should-clear logic out of clearPartitions.

[David Lehman <dlehman redhat com>]

On Thu, 2009-04-30 at 17:14 -0400, Chris Lumens wrote:
> ---
>  storage/partitioning.py |   59 ++++++++++++++++++++++++-----------------------
>  1 files changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/storage/partitioning.py b/storage/partitioning.py
> index 663d1f1..331b339 100644
> --- a/storage/partitioning.py
> +++ b/storage/partitioning.py
> @@ -249,6 +249,34 @@ def doAutoPartition(anaconda):
>          anaconda.id.storage.reset()
>          return DISPATCH_BACK
>  
> +def shouldClear(part, clearPartType, clearPartDisks=None, protectedPartitions=None):
> +    if not isinstance(part, PartitionDevice):
> +        return False
> +
> +    # If we got a list of disks to clear, make sure this one's on it
> +    if clearPartDisks and part.disk.name not in clearPartDisks:
> +        return False
> +
> +    # Don't clear partitions holding install media.
> +    if protectedPartitions and part.name in protectedPartitions:
> +        return False
> +
> +    # We don't want to fool with extended partitions, freespace, &c
> +    if part.partType not in [parted.PARTITION_NORMAL, parted.PARTITION_LOGICAL]:
> +        return False
> +
> +    if clearPartType != CLEARPART_TYPE_ALL:
> +        if not part.format or not part.format.linuxNative:

This should probably be "if not part.format.type" -- the Device classes
are rigged such that it's nearly impossible to not a have a DeviceFormat
instance as the format attribute.

> +            return False
> +
> +        if not part.partedPartition.getFlag(parted.PARTITION_LVM) and \
> +           not part.partedPartition.getFlag(parted.PARTITION_RAID) and \
> +           not part.partedPartition.getFlag(parted.PARTITION_SWAP):
> +            return False

Something's wrong here. This will return False for any formatting that
isn't mdraid, swap, or lvm (eg: all filesystems).

I think if you combine the two conditionals from inside this block
you'll be all set.

> +
> +    # TODO: do platform-specific checks on ia64, pSeries, iSeries, mac
> +
> +    return True
>  
>  def clearPartitions(storage):
>      """ Clear partitions and dependent devices from disks.
> @@ -276,35 +304,8 @@ def clearPartitions(storage):
>      clearparts = [] # list of partitions we'll remove
>      for part in partitions:
>          log.debug("clearpart: looking at %s" % part.name)
> -        clear = False   # whether or not we will clear this partition
> -
> -        # if we got a list of disks to clear, make sure this one's on it
> -        if storage.clearPartDisks and \
> -           part.disk.name not in storage.clearPartDisks:
> -            continue
> -
> -        # don't clear partitions holding install media
> -        if part.name in storage.protectedPartitions:
> -            continue
> -
> -        # we don't want to fool with extended partitions, freespace, &c
> -        if part.partType not in (parted.PARTITION_NORMAL,
> -                                 parted.PARTITION_LOGICAL):
> -            continue
> -
> -        if storage.clearPartType == CLEARPART_TYPE_ALL:
> -            clear = True
> -        else:
> -            if part.format and part.format.linuxNative:
> -                clear = True
> -            elif part.partedPartition.getFlag(parted.PARTITION_LVM) or \
> -                 part.partedPartition.getFlag(parted.PARTITION_RAID) or \
> -                 part.partedPartition.getFlag(parted.PARTITION_SWAP):
> -                clear = True
> -
> -        # TODO: do platform-specific checks on ia64, pSeries, iSeries, mac
> -
> -        if not clear:
> +        if not shouldClear(part, storage.clearPartType, storage.clearPartDisks,
> +                           storage.protectedPartitions):
>              continue
>  
>          log.debug("clearing %s" % part.name)