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

David Lehman dlehman at redhat.com
Fri Aug 10 18:34:40 UTC 2012


On Fri, 2012-08-10 at 10:56 -0700, Brian C. Lane wrote:
> 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)

It can only end up as one of the following:

 device (ActionCreateDevice, ActionDestroyFormat)
 device, format (ActionCreateFormat)
 device, targetSize (ActionResizeDevice, ActionResizeFormat)

IOW it could be if..elif..elif instead of if..if..if but there's not
really any specific reason it needs to be exclusive for the time being.

> 
> 
> 
> > +        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.

I can add a TODO.

> 
> 
> > +        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.

Sure.

> 
> 
> > +
> > +        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.

It would be good, but I already have too much on my list. Converting all
of storage is going to be a substantial task, I think. The conversion in
newPartition gets removed in this patch. Since Size usage is mostly
isolated in the ui it made sense to handle converting to float in the ui
instead of in newDevice.

> 
> > +    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.

That's an artifact of an earlier revision. Good catch.

> 
> > +
> > +        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")
> 
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches




More information about the anaconda-patches mailing list