[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH 1/5] Use a function to add a device to the partition gui.



Hi,

Patch 1/2 look good.

Patch 3:
1) now that DMRaidArrayDevice inherits from DMDevice, its
updateSysfsPath() method can be removed (like you are already doing
with MultipathDevice).

2) by changing DMRaidArrayDevice and MultipathDevice to derive
from DMDevice you are changing the way their status property works,
we need to keep a look out for regressions caused by this

3) You change DMRaidArrayDevice and MultipathDevice to derive
from DMDevice, but they are still calling the
DiskDevice.__init__ from their __init__, this will cause
self.target and self.dmUuid to not get set, which in turn
will cause a backtrace when DMDevice.__str__ gets
called

4) devicetree.py:

@@ -1250,7 +1250,7 @@ class DeviceTree(object):
         # The first step is to either look up or create the device
         #
         if udev_device_is_multipath_member(info):
-            device = StorageDevice(name,
+            device = DiskDevice(name,
                             major=udev_device_get_major(info),
                             minor=udev_device_get_minor(info),
                             sysfsPath=sysfs_path, exists=True,
@@ -1282,14 +1282,9 @@ class DeviceTree(object):
             if device is None:
                 device = self.addUdevOpticalDevice(info)
         elif udev_device_is_biosraid(info) and udev_device_is_disk(info):
-            # This is special handling to avoid the "unrecognized disklabel"
-            # code since biosraid member disks won't have a disklabel. We
-            # use a StorageDevice because DiskDevices need disklabels.
-            # Quite lame, but it doesn't matter much since we won't use
-            # the StorageDevice instances for anything.
             log.debug("%s is part of a biosraid" % name)
             if device is None:
-                device = StorageDevice(name,
+                device = DiskDevice(name,
                                 major=udev_device_get_major(info),
                                 minor=udev_device_get_minor(info),
                                 sysfsPath=sysfs_path, exists=True)

This is wrong, now handleUdevDiskLabelFormat() will get called on them
which in case of a multipath or a RAID mirror will succeed (and in
case of a RAID stripe might succeed too), resulting in the raw disk
becoming part of storage.partitioned. Maybe partionable should
become a parameter of the DiskDevice constructor and get set to
False here ?

Even with this fixed, the raw disks now will still become part of
storage.disks, and I'm not sure we even want that.


Patch 4 looks good

Patch 5, do we want to hide devices with a disklabel on them (iow normal disks)
from the main partition UI ?


Regards,

Hans


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]