On Fri, Mar 06, 2009 at 10:38:26AM -0600, David Lehman wrote:
>
> Overall it is good, but there are a couple of things you need to fix.
> See below.
>
> > diff --git a/storage/devices.py b/storage/devices.py
> > index 84cfe1b..dd5a4f1 100644
> > --- a/storage/devices.py
> > +++ b/storage/devices.py
> > @@ -2069,41 +2152,130 @@ class MDRaidArrayDevice(StorageDevice):
> > self.exists = False
> >
> >
> > -class DMRaidArrayDevice(DMDevice):
> > +class DMRaidArrayDevice(DiskDevice):
> > """ A dmraid (device-mapper RAID) device """
> > _type = "dm-raid array"
> > _packages = ["dmraid"]
> > + devDir = "/dev/mapper"
> >
> > - def __init__(self, name, format=None, size=None,
> > - exists=None, parents=None, sysfsPath=''):
> > + def __init__(self, name, raidSet=None, level=None, format=None, size=None,
> > + major=None, minor=None, parents=None, sysfsPath=''):
> > """ Create a DMRaidArrayDevice instance.
> >
> > Arguments:
> >
> > - name -- the device name (generally a device node's basename)
> > + name -- the dmraid name also the device node's basename
> >
> > Keyword Arguments:
> >
> > + raidSet -- the RaidSet object from block
> > + level -- the type of dmraid
> > parents -- a list of the member devices
> > sysfsPath -- sysfs device path
> > size -- the device's size
> > format -- a DeviceFormat instance
> > - exists -- indicates whether this is an existing device
> > """
> > if isinstance(parents, list):
> > for parent in parents:
> > if not parent.format or parent.format.type != "dmraidmember":
> > raise ValueError("parent devices must contain dmraidmember format")
> > - DMDevice.__init__(self, name, format=format, size=size,
> > - parents=parents, sysfsPath=sysfsPath,
> > - exists=exists)
> > + DiskDevice.__init__(self, name, format=format, size=size,
> > + major=major, minor=minor,
> > + parents=parents, sysfsPath=sysfsPath)
> >
> > - def probe(self):
> > - """ Probe for any missing information about this device.
> > + self.formatClass = get_device_format_class("dmraidmember")
> > + if not self.formatClass:
> > + raise DeviceError("cannot find class for 'dmraidmember'")
> >
> > - size
> > +
> > + self._raidSet = raidSet
> > + self._level = level
> > +
> > + @property
> > + def raidSet(self):
> > + return self._raidSet
> > +
> > + @raidSet.setter
> > + def raidSet(self, val):
> > + # If we change self._raidSet, parents list will be invalid.
> > + # Don't allow the change.
> > + pass
>
> I still think you should just not define this setter -- how is silently
> ignoring things better than pointing out the error? This will mask
> programming errors.
I blame my brain that did not cope with all the action :) will change.
>
> > diff --git a/storage/devicetree.py b/storage/devicetree.py
> > index 3494d5a..a83279d 100644
> > --- a/storage/devicetree.py
> > +++ b/storage/devicetree.py
> > @@ -914,8 +945,38 @@ class DeviceTree(object):
> > parents=[device])
> > self._addDevice(md_array)
> > elif format.type == "dmraidmember":
> > - # look up or create the dmraid array
> > - pass
> > + major = udev_device_get_major(info)
> > + minor = udev_device_get_minor(info)
> > + # Have we already created the DMRaidArrayDevice?
> > + rs = block.getRaidSetFromRelatedMem(uuid=uuid, name=name,
> > + major=major, minor=minor)
> > + if rs is None:
> > + # FIXME: Should handle not finding a dmriad dev better
> > + pass
>
> Shouldn't this be 'return'?
This means that something very strange is going on. because we found a
dmraid device and we cannot find the dmraidArray that it relates to.
Just returning seems odd because the installation would continue without
any further action. Guess the best thing to do here is to ignore the
device and continue.
>
> > diff --git a/storage/udev.py b/storage/udev.py
> > index 6df00c8..a39b98c 100644
> > --- a/storage/udev.py
> > +++ b/storage/udev.py
> > @@ -270,4 +270,37 @@ def udev_device_get_lv_sizes(info):
> >
> > return [float(s) / 1024 for s in sizes]
> >
> > +def udev_device_is_dmraid(info):
> > + # Note that this function does *not* identify raid sets.
> > + # Tests to see if device is parto of a dmraid set.
> > + # dmraid and mdriad have the same ID_FS_USAGE string, ID_FS_TYPE has a
> > + # string that describes the type of dmraid (isw_raid_member...), I don't
> > + # want to maintain a list and mdraid's ID_FS_TYPE='linux_raid_member', so
> > + # dmraid will be everything that is raid and not linux_raid_member
> > + if info.has_key("ID_FS_USAGE") and info.has_key("ID_FS_TYPE") and \
> > + info["ID_FS_USAGE"] == "raid" and \
> > + info["ID_FS_TYPE"] != "linux_raid_member":
> > + return True
>
> As I said before, this will identify LVM PVs as dmraid devices. This has
> to be fixed.
ok, will use the list and will compare that to the list that the
DMRaidArrayDevice class has. For now, it will be the solution.
Another patch in a bit
>
>
> _______________________________________________
> 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
