[PATCH 11/21] Use a copy of the main Storage instance during custom partitioning.

Brian C. Lane bcl at redhat.com
Fri Aug 10 17:56:54 UTC 2012


On Thu, Aug 09, 2012 at 02:28:30PM -0500, David Lehman wrote:
> +class UIStorageFilter(logging.Filter):
> +    def filter(self, record):
> +        record.name = "storage.ui"
> +        return True
> +
> + at contextmanager
> +def ui_storage_logger():
> +    storage_log = logging.getLogger("storage")
> +    f = UIStorageFilter()
> +    storage_log.addFilter(f)
> +    yield
> +    storage_log.removeFilter(f)
> +

I really like this, good solution to the logging problem.

> +            args = [device]
> +            if ui_action.isResize:
> +                args.append(ui_device.targetSize)
> +
> +            if ui_action.isCreate and ui_action.isFormat:
> +                args.append(ui_device.format)

[snip]

> +            log.debug("duplicating action '%s'" % ui_action)
> +            action = ui_action.__class__(*args)

How are the args supposed to work, the result could end up with (device,
targetSize) or (device, format) or (device, targetSize, format)



> +        if partition_creates or partition_destroys:
> +            try:
> +                doPartitioning(self.storage)
> +            except Exception as e:
> +                raise

Why wrap this in a try and then catch everything only to re-raise it?
Anticipating error handling? If so then a TODO would be good.


> +        device_type = self.data.autopart.type
> +        if device_type == AUTOPART_TYPE_LVM and \
> +             mountpoint == "/boot/efi":
> +            device_type = AUTOPART_TYPE_PLAIN
> +        elif device_type == AUTOPART_TYPE_BTRFS and \
> +             fstype == "swap" or \
> +             mountpoint and mountpoint.startswith("/boot"):
> +            device_type = AUTOPART_TYPE_PLAIN

This 'and or and' block could use some () just for clarity.


> +
> +        disks = self._clearpartDevices
> +
> +        with ui_storage_logger():
> +            self.__storage.newDevice(device_type,
> +                                     size=float(size.convertTo(spec="mb")),
> +                                     fstype=fstype,
> +                                     mountpoint=mountpoint,
> +                                     encrypted=encrypted,
> +                                     disks=disks)
> +        self._devices = self.__storage.devices
> +        self._do_refresh()
> +        self._updateSpaceDisplay()

It would be good if we could eliminate all uses of float and just use
Size everywhere. Also, in this case it is converting to float here and
then again inside newDevice it does the same conversion.

> +    def _do_autopart(self):
> +        # There are never any non-existent devices around when this runs.
> +        disks = self._clearpartDevices
> +        partitions = [d for d in self._devices if d.type == "partition"]
> +        vgs = [d for d in self._devices if d.type == "lvmvg"]

partitions and vgs never get used below.

> +
> +        log.debug("running automatic partitioning")
> +        self.__storage.doAutoPart = True
> +        with ui_storage_logger():
> +            doAutoPartition(self.__storage, self.data)
> +        self.__storage.doAutoPart = False
> +        self._devices = self.__storage.devices
> +        log.debug("finished automatic partitioning")


-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 482 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/anaconda-patches/attachments/20120810/6c140b1d/attachment.sig>


More information about the anaconda-patches mailing list