Re: [PATCH] Implement the format disk question as a callback.

[Chris Lumens <clumens redhat com>]

>  storage/devices.py    |   23 +++++++++++++--------
>  storage/devicetree.py |   50 ++++++++++++++++++++++++------------------------
>  storage/errors.py     |    3 ++
>  3 files changed, 42 insertions(+), 34 deletions(-)

Yes, I definitely think this patch is an improvement.  Comments to
follow inline...

> @@ -568,8 +569,8 @@ class DiskDevice(StorageDevice):
>      _type = "disk"
>  
>      def __init__(self, device, format=None,
> -                 size=None, major=None, minor=None,
> -                 sysfsPath='', parents=None, init = False, labeltype = None):
> +                 size=None, major=None, minor=None, sysfsPath='', \
> +                 parents=None, initcb=None, kwargsinitcb=None):
>          """ Create a DiskDevice instance.
>  
>              Arguments:
> @@ -586,8 +587,8 @@ class DiskDevice(StorageDevice):
>                  parents -- a list of required Device instances
>                  removable -- whether or not this is a removable device
>  
> -                init -- initialize label on this drive
> -                labeltype -- type of the label to use during initialization
> +                initcb -- the call back to be used when initiating disk.
> +                kwargs -- arguments for the intcb.  If no args then = {}
>  
>  
>              DiskDevices always exist.

I'd get rid of the kwargsinitcb parameter here and treat initcb as a lambda.
Also, there's no need for a slash in the arguments list.

> @@ -603,12 +604,16 @@ class DiskDevice(StorageDevice):
>          if not self.partedDevice:
>              raise DeviceError("cannot find parted device instance")
>          log.debug("creating parted Disk: %s" % self.path)
> -        if init:
> -            self.partedDisk = parted.freshDisk(device=self.partedDevice, ty = labeltype)
> -        else:
> +        try:
>              self.partedDisk = parted.Disk(device=self.partedDevice)
> -        if not self.partedDisk:
> -            raise DeviceError("failed to create parted Disk")
> +        except:
> +            # if we have a cb function use it. else an error.
> +            if initcb is not None and kwargsinitcb is not None and \
> +                    initcb(**kwargsinitcb):
> +                self.partedDisk = parted.freshDisk(device=self.partedDevice, \
> +                        ty = platform.getPlatform(None).diskType)
> +            else:
> +                raise DeviceUserDeniedFormatError("User prefered to not format.")
>  
>          self.probe()
>  

Then here, you just call initcb() instead of messing with kwargsinitcb
as well.

> @@ -749,29 +766,12 @@ class DeviceTree(object):
>                      device = DiskDevice(name,
>                                      major=udev_device_get_major(info),
>                                      minor=udev_device_get_minor(info),
> -                                    sysfsPath=sysfs_path)
> +                                    sysfsPath=sysfs_path,
> +                                    initcb=questionInitializeDisk,
> +                                    kwargsinitcb={"intf":self.intf, "name":name})
>                      self._addDevice(device)
> -                except parted.IOException: #drive not initialized?
> -                    if not self.intf:
> -                        self.ignoredDisks.append(name)
> -                    else:
> -                        rc = self.intf.messageWindow(_("Warning"),
> -                                _("Error processing drive %s.\n"
> -                                  "Maybe it needs to be reinitialized."
> -                                  "YOU WILL LOSE ALL DATA ON THIS DRIVE!") % (name,),
> -                                type="custom",
> -                                custom_buttons = [ _("_Ignore drive"),
> -                                                   _("_Re-initialize drive") ],
> -                                custom_icon="question")
> -                        if rc == 0:
> -                            self.ignoredDisks.append(name)
> -                        else:
> -                            device = DiskDevice(name,
> -                                            major=udev_device_get_major(info),
> -                                            minor=udev_device_get_minor(info),
> -                                            sysfsPath=sysfs_path, init = True,
> -                                            labeltype = platform.getPlatform(None).diskType)
> -                            self._addDevice(device)
> +                except DeviceUserDeniedFormatError: #drive not initialized?
> +                    self.ignoredDisks.append(name)
>          elif udev_device_is_partition(info):
>              log.debug("%s is a partition" % name)
>              device = self.getDeviceByName(name)

And finally, your initcb parameter looks like this:

   initcb=lambda intf,name: questionInitializeDisk(intf, name)

Just a thought, anyway.  That's the kind of way I'd handle this but then
I do enjoy a little excessive use of lambdas.  I guess the main problem
I had was that you don't really need to pass the arguments around
through several different functions.  That to me clutters things up a
bit more.  But I do like how you've moved the checking into the
DiskDevice class.

- Chris