On Wed, Mar 18, 2009 at 01:29:25PM +0100, Hans de Goede wrote:
> The way we were handling updateSysfsPath in DMRaidPartitionDevice was very
> wrong, you cannot call a method of an other class on self, when you do
> not inherit from that class. Given the minimal difference between
> normal and dmraid partitions (only path and sysfsPath differ) and all the
> pain having multiple partition classes causes, this patch makes PartitionDevice
> handle both normal and dmraid partitions, and gets rid of the whole
> partition factory thing.
> ---
> storage/__init__.py | 2 +-
> storage/devices.py | 151 ++++++++++++-----------------------------------
> storage/devicetree.py | 31 +++++-----
> storage/partitioning.py | 5 +-
> 4 files changed, 57 insertions(+), 132 deletions(-)
>
> diff --git a/storage/__init__.py b/storage/__init__.py
> index b0789d6..6bb7a84 100644
> --- a/storage/__init__.py
> +++ b/storage/__init__.py
> @@ -495,7 +495,7 @@ class Storage(object):
> else:
> name = "req%d" % self.nextID
>
> - return PartitionDeviceFactory(name, *args, **kwargs)
> + return PartitionDevice(name, *args, **kwargs)
>
> def newMDArray(self, *args, **kwargs):
> """ Return a new MDRaidArrayDevice instance for configuring. """
> diff --git a/storage/devices.py b/storage/devices.py
> index 8aef711..0922778 100644
> --- a/storage/devices.py
> +++ b/storage/devices.py
> @@ -130,81 +130,6 @@ def get_device_majors():
> return majors
> device_majors = get_device_majors()
>
> -def PartitionDeviceFactory(*args, **kwargs):
> - """This will decide what PartitionDevice class will be used.
> -
> - Arguments:
> -
> - name -- the device name (generally a device node's basename)
> -
> - Keyword Arguments:
> -
> - exists -- indicates whether this is an existing device
> - format -- the device's format (DeviceFormat instance)
> -
> - For existing partitions:
> -
> - parents -- the disk that contains this partition
> - major -- the device major
> - minor -- the device minor
> - sysfsPath -- sysfs device path
> -
> - For new partitions:
> -
> - partType -- primary,extended,&c (as parted constant)
> - grow -- whether or not to grow the partition
> - maxsize -- max size for growable partitions (in MB)
> - size -- the device's size (in MB)
> - bootable -- whether the partition is bootable
> - parents -- a list of potential containing disks
> -
> - The decision will be made base on finding the disk by name
> - """
> - # FIXME: PRePBootDevice should be here somehow.!!!
> - roots = ["/dev", "/dev/mapper"]
> - # firs lets normalize the name
> - norm_name = args[0].split("/")[-1]
> -
> - # We look for the disk in /dev. From PartitionDevice its the [0] one.
> - if not kwargs.get("exists") and not kwargs.get("parents"):
> - # Cant really choose a good type of class, default to PartitionDevice
> - # This will be considered as a request.
> - return PartitionDevice(*args, **kwargs)
> - elif not kwargs.get("exists"):
> - # some requests have a disk specified
> - parents = kwargs["parents"]
> - if isinstance(parents, Device):
> - parents = [parents]
> -
> - if isinstance(parents[0], DMRaidArrayDevice):
> - return DMRaidPartitionDevice(*args, **kwargs)
> - else:
> - return PartitionDevice(*args, **kwargs)
> - else:
> - parents = kwargs["parents"]
> - if isinstance(parents, Device):
> - parents = [parents]
> - # we receive a list of parents. look for the disk that contains the name.
> - for root in roots:
> - for parent in parents:
> - path = os.path.join(root,norm_name)
> - log.debug("looking up parted Partition: %s" % path)
> - part = parent.partedDisk.getPartitionByPath(path)
> - if not part:
> - continue
> - else:
> - # we have found a part. no make the decision based on path
> - if path.startswith(roots[1]):
> - #we create a DMPartition
> - return DMRaidPartitionDevice(*args, **kwargs)
> - elif path.startswith(roots[0]):
> - # make sure that the parent is consistent
> - kwargs["parents"] = [parent]
> - return PartitionDevice(*args, **kwargs)
> -
> - # If we get here, we did not find anything.
> - log.warning("unable to find parted device for %s" % norm_name)
> - return None
looks good.
>
> class Device(object):
> """ A generic device.
> @@ -440,7 +365,7 @@ class StorageDevice(Device):
> should create a filesystem if one has been specified.
> """
> _type = "storage device"
> - devDir = "/dev"
> + _devDir = "/dev"
> sysfsBlockDir = "class/block"
> _resizable = False
>
> @@ -512,7 +437,7 @@ class StorageDevice(Device):
> @property
> def path(self):
> """ Device node representing this device. """
> - return "%s/%s" % (self.devDir, self.name)
> + return "%s/%s" % (self._devDir, self.name)
>
> def probe(self):
> """ Probe for any missing information about this device. """
> @@ -1023,6 +948,16 @@ class PartitionDevice(StorageDevice):
> self.partedPartition = None
>
> @property
> + def path(self):
> + """ Device node representing this device. """
> + if not self.parents:
> + # Bogus, but code in various places compares devices by path
> + # So we must return something unique
> + return self.name
> +
> + return "%s/%s" % (self.parents[0]._devDir, self.name)
> +
> + @property
> def partType(self):
> """ Get the partition's type (as parted constant). """
> try:
> @@ -1082,6 +1017,20 @@ class PartitionDevice(StorageDevice):
> partedPartition = property(lambda d: d._getPartedPartition(),
> lambda d,p: d._setPartedPartition(p))
>
> + def updateSysfsPath(self):
> + """ Update this device's sysfs path. """
> + log_method_call(self, self.name, status=self.status)
> + if not self.parents:
> + self.sysfsPath = ''
> +
> + elif self.parents[0]._devDir == "/dev/mapper":
> + dm_node = dm.dm_node_from_name(self.name)
> + path = os.path.join("/sys", self.sysfsBlockDir, dm_node)
> + self.sysfsPath = os.path.realpath(path)[4:]
> +
> + else:
> + StorageDevice.updateSysfsPath(self)
> +
> def dependsOn(self, dep):
> """ Return True if this device depends on dep. """
> if isinstance(dep, PartitionDevice) and dep.isExtended and self.isLogical:
> @@ -1265,16 +1214,6 @@ class PartitionDevice(StorageDevice):
>
> self.parents = [disk]
> disk.addChild()
> - if isinstance(disk, DMRaidArrayDevice):
> - # modify the partition so it can look like the DMRaidPartitionDevice.
> -
> - self._type = "dmraid partition"
> - self._packages = ["dmraid"]
> - self.devDir = "/dev/mapper"
> - self.updateSysfsPath = DMDevice.updateSysfsPath
> - self.getDMNode = DMDevice.getDMNode
I remember putting this here because when doing autopartitioning we do
not know if the partition is going to be on a DMraid or on "normal"
device.
Did you take this case into account?
ths rest seems ok.
> -
> -
>
> disk = property(lambda p: p._getDisk(), lambda p,d: p._setDisk(d))
>
> @@ -1293,7 +1232,7 @@ class PartitionDevice(StorageDevice):
> class DMDevice(StorageDevice):
> """ A device-mapper device """
> _type = "dm"
> - devDir = "/dev/mapper"
> + _devDir = "/dev/mapper"
>
> def __init__(self, name, format=None, size=None, dmUuid=None,
> target=None, exists=None, parents=None, sysfsPath=''):
> @@ -2326,7 +2265,7 @@ class DMRaidArrayDevice(DiskDevice):
> """ A dmraid (device-mapper RAID) device """
> _type = "dm-raid array"
> _packages = ["dmraid"]
> - devDir = "/dev/mapper"
> + _devDir = "/dev/mapper"
>
> def __init__(self, name, raidSet=None, level=None, format=None,
> size=None, major=None, minor=None, parents=None,
> @@ -2406,30 +2345,18 @@ class DMRaidArrayDevice(DiskDevice):
> def deactivate(self):
> self._raidSet.deactivate()
>
> - # We are more like a disk then a dmdevice, but we are still a dmdevice,
> - # so we need to "inherit" some functions from dmdevice
> def updateSysfsPath(self):
> - DMDevice.updateSysfsPath(self)
> -
> - def getDMNode(self):
> - DMDevice.getDMNode(self)
> -
> -
> -class DMRaidPartitionDevice(PartitionDevice):
> - """ A disk partition in a dmraid array, identical to a general
> - PartitionDevice, except for the device path and sysfs path
> - """
> - _type = "dmraid partition"
> - _packages = ["dmraid"]
> - devDir = "/dev/mapper"
> -
> - # We are more like a partition then a dmdevice, but we are still a dmdevice
> - # so we need to "inherit" some functions from dmdevice
> - def updateSysfsPath(self):
> - DMDevice.updateSysfsPath(self)
> + """ Update this device's sysfs path. """
> + log_method_call(self, self.name, status=self.status)
> + if not self.exists:
> + raise DeviceError("device has not been created")
>
> - def getDMNode(self):
> - DMDevice.getDMNode(self)
> + if self.status:
> + dm_node = dm.dm_node_from_name(self.name)
> + path = os.path.join("/sys", self.sysfsBlockDir, dm_node)
> + self.sysfsPath = os.path.realpath(path)[4:]
> + else:
> + self.sysfsPath = ''
>
>
> class MultipathDevice(DMDevice):
> @@ -2518,7 +2445,7 @@ class FileDevice(StorageDevice):
> This exists because of swap files.
> """
> _type = "file"
> - devDir = ""
> + _devDir = ""
>
> def __init__(self, path, format=None, size=None,
> exists=None, parents=None):
> diff --git a/storage/devicetree.py b/storage/devicetree.py
> index 3a2af94..8ba4864 100644
> --- a/storage/devicetree.py
> +++ b/storage/devicetree.py
> @@ -897,16 +897,11 @@ class DeviceTree(object):
> udev_device_is_dmraid_partition(info, self):
> diskname = udev_device_get_dmraid_partition_disk(info)
> disk = self.getDeviceByName(diskname)
> - device = PartitionDeviceFactory(name, \
> - sysfsPath=sysfs_path, \
> - major=udev_device_get_major(info), \
> - minor=udev_device_get_minor(info), \
> - exists=True, \
> - parents=[disk])
> - if not device:
> - return
> + device = PartitionDevice(name, sysfsPath=sysfs_path,
> + major=udev_device_get_major(info),
> + minor=udev_device_get_minor(info),
> + exists=True, parents=[disk])
> self._addDevice(device)
> - #self.addIgnoredDisk(name)
>
> # if we get here, we found all of the slave devices and
> # something must be wrong -- if all of the slaves are in
> @@ -1058,14 +1053,18 @@ class DeviceTree(object):
> log.error("failure scanning device %s" % disk_name)
> return
>
> - device = PartitionDeviceFactory(name,
> - sysfsPath=sysfs_path,
> - major=udev_device_get_major(info),
> - minor=udev_device_get_minor(info),
> - exists=True,
> - parents=[disk])
> - if not device:
> + try:
> + device = PartitionDevice(name, sysfsPath=sysfs_path,
> + major=udev_device_get_major(info),
> + minor=udev_device_get_minor(info),
> + exists=True, parents=[disk])
> + except DeviceError:
> + # corner case sometime the kernel accepts a partition table
> + # which gets rejected by parted, in this case we will
> + # prompt to re-initialize the disk, so simply skip the
> + # faulty partitions.
> return
> +
> self._addDevice(device)
>
> #
> diff --git a/storage/partitioning.py b/storage/partitioning.py
> index 8243080..92458d9 100644
> --- a/storage/partitioning.py
> +++ b/storage/partitioning.py
> @@ -32,7 +32,7 @@ from constants import *
>
> from errors import *
> from deviceaction import *
> -from devices import PartitionDeviceFactory, LUKSDevice
> +from devices import PartitionDevice, LUKSDevice
>
> import gettext
> _ = lambda x: gettext.ldgettext("anaconda", x)
> @@ -568,8 +568,7 @@ def doPartitioning(storage, exclusiveDisks=None):
> # that does not exist means leaving self.parents empty and instead
> # populating self.req_disks. In this case, we need to skip past
> # that since this partition is already defined.
> - device = PartitionDeviceFactory(extended.getDeviceNodeName(),
> - parents=disk)
> + device = PartitionDevice(extended.getDeviceNodeName(), parents=disk)
> device.parents = [disk]
> device.partedPartition = extended
> storage.createDevice(device)
> --
> 1.6.1.3
>
> _______________________________________________
> 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
