On Wed, 2009-12-09 at 12:01 +0100, Hans de Goede wrote:
> 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).
Thanks.
>
> 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
Yes, I was a little hasty doing the dmraid/mpath part. I'll take a look
at 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
Likewise.
>
> 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 ?
Instantiating DiskDevice for a disk is most certainly not wrong. I did
forget about the disklabel showing up in some cases, though -- I may
expand the partitionable attribute into a property for this.
>
> Even with this fixed, the raw disks now will still become part of
> storage.disks, and I'm not sure we even want that.
I'm not sure why we wouldn't now that the (disk ==> disklabel) rule is
gone. However, we could certainly use those formats' new "hidden"
attribute for this if we do want to filter them out any place they are
found to be problematic.
>
>
> 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 ?
Yes, because partitioned devices are handled differently than other
formatting. The "hidden" attribute only prevents the device from being
displayed in the main partitioning screen as a leaf node in the tree.
Partitioned devices, like volume groups, do not appear as leaf nodes in
the tree -- the partitions are the leaf nodes (like the logical
volumes).
Of course, if we wanted to, we could expand DiskLabel's name property so
that is says "MSDOS" or "GPT" or whatever -- that *might* be suitable
for display in the tree. I'd say probably better to leave it as-is.
I will probably post a follow-on patch to partition_gui.py that makes
the handling of partitioned -v- otherwise more generic, to account for
partitioned md and whatever else.
Thanks for the thorough review. I'll be posting updated patches --
hopefully soon.
Dave
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
