On Thu, Mar 05, 2009 at 04:17:47PM -0500, Chris Lumens wrote:
> > 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.
Good idea..
> Also, there's no need for a slash in the arguments list.
I just like to put it everywhere I jump a line.
>
> > @@ -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)
But to hard code the values I would need to do it as Radek suggested.
>
> 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
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
--
Joel Andres Granados
Brno, Czech Republic, Red Hat.
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
